Inline the copying collector's bump allocation in Wasm code

[fitzgen]

No description provided.

[github-actions]

Subscribe to Label Action

cc @fitzgen

Details

This issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types"

Thus the following users have been cc'd because of the following labels:

• fitzgen: wasmtime:ref-types

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[alexcrichton]

This is preexisting I think for other collectors, but there's 2 primary ways that this method is UB according to Miri:

1. This is deriving a pointer from &self which is later mutated through in compiled code. That's UB in Rust because by deriving the pointer from &self you're saying that the contents can't be mutated. This is a playground example showing the UB (top right, Tools -> Miri). The fix for this specific issue is to change this method to &mut self and then have the implementation be NonNull::from(&mut self.vmctx_data).cast()
2. The second problem is that this struture is stored inline with the CopyingHeap, which means that re-acquiring a mutable pointer to CopyingHeap will actually invalidate the previous pointer returned by this method. This is a playground example of the UB here.

Fixing the latter one is not going to be super easy. I can think a few possible ways:

• Every time we return back to wasm re-acquire the pointer and put it back in the VMContext. Probably too slow to work.
• Place the VM data behind an indirection (e.g. Box<VMCopyingHeapData> here). The goal is that by using &mut CopyingHeap it doesn't auto-invaildate the vmctx data structures. This alone is not enough, however. Another added fix necessary would be to create the raw pointer at creation of CopyingHeap, and then CopyingHeap itself only accesses the data through that pointer. The end result is sort of like this and models what we do with vm::Instance and its sibling VMContext storage data that comes after it.

I haven't double-checked, but I believe this would all plague the drc/null collectors as well. If you'd like I think it'd be reasonable to fix them all in a subsequent PR.

In terms of detecting all of this, in theory Miri should be able to capture everything here. I think that may mean we're not running much GC-compiled wasms through Miri, which might be a good addition to have? For miri coverage copying this line is probably the easiest. Along those lines rustup run nightly ./ci/miri-wast.sh ./tests/spec_testsuite/struct.wast locally shows a Miri violation which may be related to this, but it's always sort of hard to tell where exactly the miri error comes from due to the mix of pulley and lack of strict provenance...

[fitzgen]

Will handle this in a follow up.

[alexcrichton]

Oh, another possible fix is like Store::data_mut where provenance of some Miri-safe pointer is used but the address is used like usual, meaning that the actual machine code doesn't change. This will still require an indirection, however, I think.

[fitzgen]

FWIW this is a 2% reduction in instructions retired on splay.wasm, but I haven't been able to measure a statistically significant difference in cycles yet. Will try bumping up the number of iterations.

[fitzgen]

FWIW this is a 2% reduction in instructions retired on splay.wasm, but I haven't been able to measure a statistically significant difference in cycles yet. Will try bumping up the number of iterations.

With 200 iterations:

execution :: cycles :: benchmarks/splay/splay.wasm

  Δ = 84634629.66 ± 43938351.82 (confidence = 99%)

  inline-alloc.so (-Wgc,function-references -Ccollector=copying) is 1.00x to 1.01x faster than main.so (-Wgc,function-references -Ccollector=copying)!

  [13062747333 13278335414.74 13771371538] inline-alloc.so (-Wgc,function-references -Ccollector=copying)
  [13062358191 13362970044.40 13846668642] main.so (-Wgc,function-references -Ccollector=copying)

[alexcrichton]

Any idea what's going on with the splay benchmark? Is it not actually allocating that much? Or is something else so massively dominating that allocation isn't factoring in?

[fitzgen]

Any idea what's going on with the splay benchmark? Is it not actually allocating that much? Or is something else so massively dominating that allocation isn't factoring in?

It has a few constants that control the size of the splay tree to create, the number of splay tree mutations to perform once its created, and one other thing that I forget. When porting it to Wasm, I replaced those constants with configurable parameters read from default.input, and then I also adjusted the default inputs to make executing it take a little longer and match some of our other benchmarks. That last bit involved mostly doing 10x as many (IIRC) mutations to the tree, while keeping it the same size, so this change could have made it less allocation-heavy in an overall relative sense, and exercising GC object accesses more. (IIRC also, trying to make the tree bigger was starting to hit GC OOM).

Also, it is written in an "idiomatic" style with lots of little functions, and we don't do any inlining by default. That could probably change things drastically for it.

[fitzgen]

Any idea what's going on with the splay benchmark? Is it not actually allocating that much? Or is something else so massively dominating that allocation isn't factoring in?

FWIW, here is a quick perf profile of splay.wasm:

Nearly all time is spent inside the collector itself.