wasmtime / PR #5127 Cranelift: Use a single, shared vecto... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #5127 Cranelift: Use a single, shared vecto...

Wasmtime GitHub notifications bot (Oct 25 2022 at 22:39):

fitzgen requested cfallin for a review on PR #5127.

Wasmtime GitHub notifications bot (Oct 25 2022 at 22:39):

fitzgen opened PR #5127 from single-vec-for-abi-args to main:

Instead of two (large!!) SmallVecs per SigData.

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, and cachegrind profiles that show whether SigData 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>

Wasmtime GitHub notifications bot (Oct 25 2022 at 23:00):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Oct 25 2022 at 23:05):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 25 2022 at 23:05):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 25 2022 at 23:05):

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 how args.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 have Deref 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?

Wasmtime GitHub notifications bot (Oct 25 2022 at 23:05):

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...

Wasmtime GitHub notifications bot (Oct 26 2022 at 11:01):

bjorn3 created PR review comment:

Indeed. There is at least one tier 3 target with 16bit pointers.

Wasmtime GitHub notifications bot (Oct 26 2022 at 11:01):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 31 2022 at 20:15):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 31 2022 at 20:15):

fitzgen created PR review comment:

How strongly do you feel about this? Uses will go from args.len() to args.args().len() which I don't feel is an improvement. rust-analyzer will happily jump to [T]::len when you do jump-to-definition on args.len(), so I feel like it isn't hidden. I can definitely highlight the deref impls in the doc comment for ArgsAccumulator as well.

Wasmtime GitHub notifications bot (Oct 31 2022 at 20:17):

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 with Vec 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.

Wasmtime GitHub notifications bot (Oct 31 2022 at 20:17):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 31 2022 at 20:32):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 31 2022 at 20:32):

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 used Deref 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-line fn len(&self) { ... } that proxies to the inner slice. If that glue is more than a few methods then sure, let's have the Deref and make sure we make a note somewhere (in the main doc-comment for ArgsAccumulator maybe) that it auto-derefs to the slice.

Wasmtime GitHub notifications bot (Oct 31 2022 at 20:35):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 31 2022 at 20:35):

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.

Wasmtime GitHub notifications bot (Oct 31 2022 at 20:40):

fitzgen updated PR #5127 from single-vec-for-abi-args to main.

Wasmtime GitHub notifications bot (Oct 31 2022 at 20:53):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Oct 31 2022 at 20:53):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Oct 31 2022 at 20:53):

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 {

Wasmtime GitHub notifications bot (Oct 31 2022 at 21:32):

fitzgen merged PR #5127.

Last updated: Oct 23 2024 at 20:03 UTC