fitzgen requested cfallin for a review on PR #5127.
fitzgen opened PR #5127 from single-vec-for-abi-args
to main
:
Instead of two (large!!)
SmallVec
s perSigData
.Unfortunately, I haven't measured any perf benefits from this, but we did shrink the size of
SigData
from 632 bytes to 56 bytes! It could probably get even smaller still, but I'm going to hold off on that until I get some new DHAT,perf
, andcachegrind
profiles that show whetherSigData
is still important to spend time on or not.Before:
struct cranelift_codegen::machinst::abi::SigData size: 632 members: 0[296] args: struct smallvec::SmallVec<[cranelift_codegen::machinst::abi::ABIArg; 6]> 296[296] rets: struct smallvec::SmallVec<[cranelift_codegen::machinst::abi::ABIArg; 6]> 592[8] sized_stack_arg_space: i64 600[8] sized_stack_ret_space: i64 608[16] stack_ret_arg: struct core::option::Option<usize> 624[1] call_conv: enum cranelift_codegen::isa::call_conv::CallConv 625[7] <padding>
After:
struct cranelift_codegen::machinst::abi::SigData size: 56 members: 0[8] sized_stack_arg_space: i64 8[8] sized_stack_ret_space: i64 16[16] stack_ret_arg: struct core::option::Option<usize> 32[8] arg_indices: struct core::ops::range::Range<u32> 40[8] ret_indices: struct core::ops::range::Range<u32> 48[1] call_conv: enum cranelift_codegen::isa::call_conv::CallConv 49[7] <padding>
jameysharp submitted PR review.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
I may be in the minority opinion on this but I have a slight preference against too much magic
Deref
: it took me a bit too long to figure out howargs.len()
was working (answer:Deref
lets this become a slice and then.len()
works on that). If so for me then doubly so for someone new to our codebase trying to learn how things work. I sort of expect smart pointer types to haveDeref
but I don't expect that for an "accumulator"!Could we have a
.args()
and.args_mut()
or similar to make it clear what's going on instead?
cfallin created PR review comment:
Huh, I'm surprised there's no
From<u32> for usize
; I guess this is in case we have 16-bit pointers...
bjorn3 created PR review comment:
Indeed. There is at least one tier 3 target with 16bit pointers.
bjorn3 submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
How strongly do you feel about this? Uses will go from
args.len()
toargs.args().len()
which I don't feel is an improvement.rust-analyzer
will happily jump to[T]::len
when you do jump-to-definition onargs.len()
, so I feel like it isn't hidden. I can definitely highlight the deref impls in the doc comment forArgsAccumulator
as well.
fitzgen created PR review comment:
I sort of expect smart pointer types to have
Deref
I feel this is exactly as much of a smart pointer as
Vec
itself is, so I'd argue that if you're okay withVec
dereferencing to a slice, than this should be okay too.but I don't expect that for an "accumulator"!
Happy to bikeshed the name as well, I put approximately zero thought into it.
fitzgen submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
Yeah, I don't feel too strongly about it, I just have a general discomfort with implicit magic: it imposes a cost on the reader to maintain some mental state when reading code. The most frustrating codebases for me to dive into are those where there is a lot of magic dispatch and implicit connection; I don't like to weave a maze that will confuse others like that.
(Also, I'd prefer not to have a codebase that requires
rust-analyzer
to comprehend well; not everyone has it installed, it can be frustrating to set up, it's fairly resource-intensive, etc.)I actually didn't realize
Vec
usedDeref
to "inherit" all the methods on slices; that's good to know. I do feel that's a slightly different case though, as it's part of the standard library and part of the shared base of knowledge most Rust programmers will have; here we are defining a custom thing.If we're using just a few methods from the slice API (is it just
.len()
that uses this in practice?) I'd prefer to hand-write the 3-linefn len(&self) { ... }
that proxies to the inner slice. If that glue is more than a few methods then sure, let's have theDeref
and make sure we make a note somewhere (in the main doc-comment forArgsAccumulator
maybe) that it auto-derefs to the slice.
fitzgen submitted PR review.
fitzgen created PR review comment:
Some of the backends (s390x iirc) will also index into the slice and reassign args within slots. I don't feel super strongly so I'll just add an
args
method.
fitzgen updated PR #5127 from single-vec-for-abi-args
to main
.
jameysharp submitted PR review.
jameysharp submitted PR review.
jameysharp created PR review comment:
This was pre-existing, but I don't see any reason this loop can't be a simple iterator:
for arg in args.args_mut() { match arg {
fitzgen merged PR #5127.
Last updated: Dec 23 2024 at 12:05 UTC