wasmtime / PR #10503 Reuse Wasm linear memories code for ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #10503 Reuse Wasm linear memories code for ...

Wasmtime GitHub notifications bot (Apr 01 2025 at 18:28):

fitzgen requested alexcrichton for a review on PR #10503.

Wasmtime GitHub notifications bot (Apr 01 2025 at 18:28):

fitzgen opened PR #10503 from fitzgen:gc-heap-config-knobs to bytecodealliance:main:

Instead of bespoke code paths and structures for Wasm GC, this commit makes it so that we now reuse VM structures like VMMemoryDefinition and bounds-checking logic. Notably, we also reuse all the associated bounds-checking optimizations and, when possible, virtual-memory techniques to completely elide them.

Furthermore, this commit adds support for growing GC heaps, reusing the machinery for growing memories, and makes it so that GC heaps always start out empty. This allows us to properly delay allocating the GC heap's storage until a GC object is actually allocated.

Fixes #9350

Wasmtime GitHub notifications bot (Apr 01 2025 at 18:28):

fitzgen requested cfallin for a review on PR #10503.

Wasmtime GitHub notifications bot (Apr 01 2025 at 18:28):

fitzgen requested wasmtime-core-reviewers for a review on PR #10503.

Wasmtime GitHub notifications bot (Apr 01 2025 at 18:28):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #10503.

Wasmtime GitHub notifications bot (Apr 01 2025 at 18:30):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

github-actions[bot] commented on PR #10503:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton submitted PR review:

I've gotten through a chunk of this, but if possible I do really think it would be nice to split this apart. I realize it's extra work over what's already here to split this apart but it's pretty hard to review this all-at-once even though I was helping out for at least a chunk of it. If it's too hard it doesn't have to be split but I tend to get nervous when big changes to core runtime bits happen all in one large PR

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton created PR review comment:

Note to myself: somehow I think we need to address this before merging. At minimum this should probably have a FIXME (or somewhere else), but changing all embeddings to reserve two slots for memory when basically no embedding using GC right now feels pretty bad as well.

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton created PR review comment:

I see these both. here and in wasmtime-wast, could they perhaps move to helper methods directly on StoreContextMut or similar?

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton created PR review comment:

TODO (and just below)

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton created PR review comment:

Mind leaving this as NonNull to avoid the cognitive overhead of using SendSyncPtr?

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton created PR review comment:

TODO

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton created PR review comment:

The ErrorExt trait I added in a PR yesterday means I think we can avoid this refactoring and/or you can use assert_contains instead of manually doing e:?

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton created PR review comment:

I think this line of the error message can be omitted as assert! should print out the expression that failed

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton created PR review comment:

Similar to tests, this pattern is 3 times in this file, would it be possible to refactor that to share code amongst the users?

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton created PR review comment:

Ditto from libcalls, this is being duplicated enough that it should be centralized somehow to a smaller handful of functions than are in use today

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton created PR review comment:

TODO

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton created PR review comment:

TODO

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton created PR review comment:

Should this be inverted? First GC then maybe grow if necessary?

Wasmtime GitHub notifications bot (Apr 01 2025 at 19:44):

alexcrichton created PR review comment:

And these should return an error if bytes_needed space wasn't actually free'd?

Wasmtime GitHub notifications bot (Apr 07 2025 at 21:54):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 07 2025 at 22:08):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 07 2025 at 22:11):

fitzgen commented on PR #10503:

Okay, after splitting out all of those PRs, I have this down to
51 files changed, 1650 insertions(+), 698 deletions(-)
when ignoring the disas test expectation updates, which isn't too bad.

Still need to fix the C API and address some of the review comments. Will leave a comment once I've done that and this is ready for re-review.

Wasmtime GitHub notifications bot (Apr 08 2025 at 20:37):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 08 2025 at 20:37):

fitzgen created PR review comment:

This is a little bit of an annoying combinatorial problem:

we want async and regular versions

we want an externref version that plumbs through the external value to the retry, and a non-externref version that doesn't have that value

we want this method on all of Store, StoreContextMut, and Caller

And even after all that, the API is still not very good, because as a caller you don't actually want to think about this stuff, you just want to do ExternRef::new(...) and have everything else just work.

We originally didn't automatically trigger GC because that means we would need both new and new_async methods on {Any,Extern,...}Ref, as well as new_manually_rooted and new_manually_rooted_async. But I'm starting to think that is actually better than trying to expose generic retry_after_gc methods. And I also don't think we actually need new_manually_rooted variants at all: users can easily do ExternRef::new(..).to_manually_rooted(..) if they want to. Then we only need FooRef::new and FooRef::new_async, which really isn't too bad from a maintenance perspective, and is way better from a user API perspective.

So: what do you think of doing that larger clean up after this lands?

Wasmtime GitHub notifications bot (Apr 08 2025 at 20:52):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 08 2025 at 20:52):

fitzgen created PR review comment:

Ah, this isn't necessary anymore. Removed.

Wasmtime GitHub notifications bot (Apr 08 2025 at 21:49):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 08 2025 at 21:49):

fitzgen created PR review comment:

Unfortunately, if you specify any custom message, then it doesn't show the original expression anymore: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=fb30232a93261b5bfa0bd219eac3ea7c

Wasmtime GitHub notifications bot (Apr 08 2025 at 22:02):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 08 2025 at 22:02):

fitzgen created PR review comment:

We want to grow first because
it is usually cheaper (just updating the accessible region of a mapping; even copying bytes to a new region is also cheaper) and
if we swapped the order, then we could hit the following pathological case:

```rust
// Assume: the GC heap has capacity for N objects, and we have N-1
// objects allocated and held alive.

// Allocate an object, now the GC heap is at capacity. We do not keep the
// object live, however.
drop(allocate());

loop {
// Upon entry of this loop, the GC heap is at capacity.
 // Allocate another object. This triggers a collect-or-else-grow, the
 // collection reclaims the previously allocated object, and then this
 // allocation succeeds without needing to grow the heap.
 drop(allocate());

 // At the end of this loop, the GC heap is still at capacity.
}
```

This scenario means we will trigger a GC on every allocation inside the loop which is super pathological. We should really only enter into this kind of behavior when there really is no more memory we can give the GC heap (and even then, the embedder probably wants to cancel the execution via async epochs/fuel at some point; may eventually make sense to add a GC limiter style API).

Wasmtime GitHub notifications bot (Apr 08 2025 at 22:05):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 08 2025 at 22:05):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 08 2025 at 22:05):

fitzgen created PR review comment:

See my other comment about folding the retry-after-gc into the FooRef constructors.

Wasmtime GitHub notifications bot (Apr 08 2025 at 22:07):

fitzgen commented on PR #10503:

Okay I've fixed the C API's compilation and updated most things pointed out in review, modulo making a single shared retry-after-gc helper that we expose from the wasmtime crate. See my comments about that here: https://github.com/bytecodealliance/wasmtime/pull/10503#discussion_r2033983294

I think this is ready for another round of review @alexcrichton.

Wasmtime GitHub notifications bot (Apr 08 2025 at 22:46):

fitzgen requested alexcrichton for a review on PR #10503.

Wasmtime GitHub notifications bot (Apr 08 2025 at 22:46):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 08 2025 at 22:46):

fitzgen requested wasmtime-fuzz-reviewers for a review on PR #10503.

Wasmtime GitHub notifications bot (Apr 08 2025 at 22:52):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 08 2025 at 23:03):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 08 2025 at 23:16):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 09 2025 at 00:39):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 09 2025 at 02:15):

github-actions[bot] commented on PR #10503:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing", "wasmtime:c-api"

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

fitzgen: fuzzing

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

Learn more.
</details>

Wasmtime GitHub notifications bot (Apr 09 2025 at 15:02):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 09 2025 at 15:02):

alexcrichton created PR review comment:

That makes sense, but isn't this current behavior pathological too? GC heaps will, by default, balloon to 4G before a GC ever runs?

This makes me think that there's not an ideal answer one way or the other, but picking either extreme seems bad?

Wasmtime GitHub notifications bot (Apr 09 2025 at 15:04):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 09 2025 at 15:04):

alexcrichton created PR review comment:

Agreed with your conclusions as well. And yeah ok let's defer this for dual new and new_async constructors.

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:00):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:00):

alexcrichton created PR review comment:

This carries an unsafe requirement that the memory being replaced has to be basically the same as take_memory, right? In that it must be at least the same size?

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:00):

alexcrichton created PR review comment:

re my comment below: this was my worry, the method here duplicates maybe_async_grow_or_collect_gc_heap and I find it confusing to trace what method is used where. I'd prefer ideally if there were a single "do gc" method that everything funnels through rather than having a forest of methods with no narrow waist

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:00):

alexcrichton created PR review comment:

Can you add some comments here for why the unreachable!() isn't reachable? It wasn't immediately obvious to me until I saw that the new constructor is gated, so presumably it's not possible to call this method when gc is disabled, but I think a comment here might be good to have

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:00):

alexcrichton created PR review comment:

Outdated change now? (although I understand why this was done I think https://github.com/bytecodealliance/wasmtime/pull/10538 is the "true" fix? or something like that?)

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:00):

alexcrichton created PR review comment:

Why not define this as size_of::<VMGcKind>() to avoid the const assertion?

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:00):

alexcrichton created PR review comment:

Would it be possible to avoid the split between either GC or grow? My hope would be that there's a single location which makes a determination of what to do. I might just not be familiar with the code and the method below is The Location, but I feel like the logic of exactly whether to grow or GC is spread out across multiple files at this time.

I understand we probably want a method-per-op at some point but I would hope that would be buried in some guaranteed-module-private location rather than so prominently exposed to the entire codebase through the VMStore trait.

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:00):

alexcrichton created PR review comment:

To double-check, you've confirmed that all locations that evaluate const exprs are on fibers?

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:00):

alexcrichton created PR review comment:

Bikeshedding a bit here, how about StaticOffset, StaticObjectField, and DynamicObjectField? I know you're in a GC-centric mindset right now but making normal linear memory accesses named BoundsCheck::Field feels a bit weird to me

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:00):

alexcrichton created PR review comment:

AFAIK this is the first time this code has had to deal with access_size == 0, could you do an audit on the below function to ensure that everything works reasonably if the access size is zero?

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:00):

alexcrichton created PR review comment:

Mind renaming s/gc_heap_bound/bound/? (to emphasize this isn't just GC accesses)

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:00):

alexcrichton created PR review comment:

(un-resolving this since this is still SendSyncPtr?)

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:10):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:10):

fitzgen created PR review comment:

Oh huh, I guess I lost this in a rebase or something, because I definitely undid this change at one point

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:19):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 09 2025 at 16:20):

fitzgen commented on PR #10503:

I'm going to implement the new and new_async stuff first, before this PR lands, because I am finding a long tail of ref-type allocations in things like doc tests, and fixing all of them is annoying. Would rather just fix the callee than every caller.

Wasmtime GitHub notifications bot (Apr 09 2025 at 20:48):

fitzgen commented on PR #10503:

I'm going to implement the new and new_async stuff first, before this PR lands, because I am finding a long tail of ref-type allocations in things like doc tests, and fixing all of them is annoying. Would rather just fix the callee than every caller.

This is https://github.com/bytecodealliance/wasmtime/pull/10560

Wasmtime GitHub notifications bot (Apr 10 2025 at 20:07):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 10 2025 at 20:07):

fitzgen created PR review comment:

Yep :+1:

Wasmtime GitHub notifications bot (Apr 11 2025 at 18:43):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 11 2025 at 18:43):

fitzgen created PR review comment:

The idea is to do it this way to make sure that it doesn't actually vary depending on the target

Wasmtime GitHub notifications bot (Apr 11 2025 at 18:52):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 11 2025 at 18:52):

fitzgen created PR review comment:

looks good to me

Wasmtime GitHub notifications bot (Apr 11 2025 at 19:16):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 11 2025 at 19:34):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 11 2025 at 19:40):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 11 2025 at 19:40):

fitzgen created PR review comment:

I think growing to the limit before doing GC is actually what you want: you don't want to do GC if you can avoid it. Growing the heap is faster than doing a GC. There are knobs for limiting the max size of memories, including the GC heap, so unbounded growth isn't really a concern (also there is a hard bound of 4 GiB since the GC heap is a 32-bit memory). And we are likely going to throw away the whole store, and the GC heap along with it, before we ever need to collect, and doing that is also much faster than doing a collection.

Wasmtime GitHub notifications bot (Apr 11 2025 at 19:41):

fitzgen requested alexcrichton for a review on PR #10503.

Wasmtime GitHub notifications bot (Apr 11 2025 at 19:44):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 11 2025 at 19:58):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 11 2025 at 21:02):

fitzgen updated PR #10503.

Wasmtime GitHub notifications bot (Apr 11 2025 at 21:03):

fitzgen has enabled auto merge for PR #10503.

Wasmtime GitHub notifications bot (Apr 11 2025 at 21:39):

fitzgen merged PR #10503.

Last updated: Apr 17 2025 at 07:03 UTC