wasmtime / PR #11514 GC: replace ManuallyRooted with Owne... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #11514 GC: replace ManuallyRooted with Owne...

Wasmtime GitHub notifications bot (Aug 22 2025 at 22:24):

cfallin requested fitzgen for a review on PR #11514.

Wasmtime GitHub notifications bot (Aug 22 2025 at 22:24):

cfallin opened PR #11514 from cfallin:owned-rooted-gc to bytecodealliance:main:

This implements the ideas from #11445: it replaces ManuallyRooted, which requires an explicit unroot action with a mut borrow of the store (making it impossible to implement in a standard Drop impl), with OwnedRooted, which holds an Arc only to a small auxiliary memory allocation (an Arc<()>) and uses this externalized "liveness flag" to allow for a Store-less drop. These liveness flags are scanned during a "trim" pass, which happens both when new owned roots are created, and just before a GC.

This should greatly increase safety for host-side users of GC: it provides a way to have a handle whose ownership works like any other Rust value, alive until dropped. It is still not quite as efficient as LIFO-scoped handles (by analogy, for the same reason that individually-freed RAII types are not as efficient as arena allocation), so those remain for efficiency-minded users that have a clear picture of reference lifetimes.

At some later time we may wish to use OwnedRooted exclusively rather than Rooted, and we may wish to rename Rooted to ScopedRooted, but I haven't done either of those things yet.

I opted to replace ManuallyRooted rather than add a third kind of root, after discussion with fitzgen. One implication of this is that the C API's anyref and externref types are now 24 or 20 bytes rather than 16 (because of the Arc pointer), and correspondingly the Val union grew to that size. I believe this is an acceptable tradeoff, but I'm happy to put ManuallyRooted back if not.

Fixes #11445.

Wasmtime GitHub notifications bot (Aug 22 2025 at 22:24):

cfallin requested wasmtime-core-reviewers for a review on PR #11514.

Wasmtime GitHub notifications bot (Aug 22 2025 at 22:26):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 22 2025 at 22:26):

cfallin edited PR #11514:

This implements the ideas from #11445: it replaces ManuallyRooted, which requires an explicit unroot action with a mut borrow of the store (making it impossible to implement in a standard Drop impl), with OwnedRooted, which holds an Arc only to a small auxiliary memory allocation (an Arc<()>) and uses this externalized "liveness flag" to allow for a Store-less drop. These liveness flags are scanned during a "trim" pass, which happens both when new owned roots are created, and just before a GC.

This should greatly increase safety for host-side users of GC: it provides a way to have a handle whose ownership works like any other Rust value, alive until dropped. It is still not quite as efficient as LIFO-scoped handles (by analogy, for the same reason that individually-freed RAII types are not as efficient as arena allocation), so those remain for efficiency-minded users that have a clear picture of reference lifetimes.

At some later time we may wish to use OwnedRooted exclusively in our public APIs rather than Rooted, and we may wish to rename Rooted to ScopedRooted, but I haven't done either of those things yet.

I opted to replace ManuallyRooted rather than add a third kind of root, after discussion with fitzgen. One implication of this is that the C API's anyref and externref types are now 24 or 20 bytes rather than 16 (because of the Arc pointer), and correspondingly the Val union grew to that size. I believe this is an acceptable tradeoff, but I'm happy to put ManuallyRooted back if not.

Fixes #11445.

Wasmtime GitHub notifications bot (Aug 22 2025 at 22:39):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 22 2025 at 22:39):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 22 2025 at 23:59):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 23 2025 at 00:06):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 23 2025 at 00:10):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 23 2025 at 00:18):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 23 2025 at 03:03):

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

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:c-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 (Aug 23 2025 at 03:33):

cfallin commented on PR #11514:

I've come up with a way to avoid the size growth on the C-API ValUnion, I think -- will update on Monday.

Wasmtime GitHub notifications bot (Aug 23 2025 at 05:11):

cfallin edited a comment on PR #11514:

~~I've come up with a way to avoid the size growth on the C-API ValUnion, I think -- will update on Monday.~~ (actually perhaps not: the idea was to put the Weak directly in the slab of owned GC ref roots, but then the trim pass requires iteration over the whole index space rather than only live roots. The original C API struct tweak is probably better than pessimizing the otherwise fairly good asymptotic behavior here for the common case)

Wasmtime GitHub notifications bot (Aug 25 2025 at 15:30):

alexcrichton commented on PR #11514:

Updating C API bits I think is fine, but natively I'm surprised that this is still using a generational index. I thought that was only necessary due to the manual nature of ManuallyRooted and for something with a dtor like this we wouldn't need generational indices? I haven't fully thought this through but that feels redundant now.

Considering the C API some more too, this'll want to go over documentation with a fine-tooth comb as well because we've probably documented "you should call unroot, but nothing bad happens if you don't so long as you free the store". Now that needs to be changed to "you must call unroot or host memory will leak". That's a pretty big departure in behavior from before, which while not bad we need to be sure is clearly communicated.

Wasmtime GitHub notifications bot (Aug 25 2025 at 15:32):

alexcrichton edited a comment on PR #11514:

Updating C API bits I think is fine, but naively I'm surprised that this is still using a generational index. I thought that was only necessary due to the manual nature of ManuallyRooted and for something with a dtor like this we wouldn't need generational indices? I haven't fully thought this through but that feels redundant now.

Considering the C API some more too, this'll want to go over documentation with a fine-tooth comb as well because we've probably documented "you should call unroot, but nothing bad happens if you don't so long as you free the store". Now that needs to be changed to "you must call unroot or host memory will leak". That's a pretty big departure in behavior from before, which while not bad we need to be sure is clearly communicated.

Wasmtime GitHub notifications bot (Aug 25 2025 at 18:10):

fitzgen created PR review comment:

Nitpick: it looks like this one line wasn't wrapped.

Wasmtime GitHub notifications bot (Aug 25 2025 at 18:10):

fitzgen created PR review comment:

Nitpick/aside: SM avoids contention/synchronization by essentially having JSContext not be Send. Our Store is Send, however -- and _must_ be for Wasmtime to work smoothly within Rust's async ecosystem -- so we would have to deal with that contention/synchronization. In practice, I doubt there would ever really be _contention_, but even the uncontended synchronization costs would be annoyingly high for us, almost definitely too high for serious consideration.

Wasmtime GitHub notifications bot (Aug 25 2025 at 18:10):

fitzgen created PR review comment:

That is, I think this can be derived now, instead of manually written out.

Wasmtime GitHub notifications bot (Aug 25 2025 at 18:10):

fitzgen created PR review comment:

    /// independently from the actual heap. Thus, we could trim before

Wasmtime GitHub notifications bot (Aug 25 2025 at 18:10):

fitzgen submitted PR review:

Fantastic! Thanks for whipping this up!

Wasmtime GitHub notifications bot (Aug 25 2025 at 18:10):

fitzgen created PR review comment:

A little surprised to see 4 here instead of 2. I guess in general I'd prefer to err on the side of smaller growth factors before the next trim to help cut down on floating bloat. But now I am wondering if there is a specific reason you have this higher?

Wasmtime GitHub notifications bot (Aug 25 2025 at 18:10):

fitzgen created PR review comment:

I think we can #[derive(Clone)] for OwnedRooted, right?

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:43):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:43):

cfallin created PR review comment:

Ah, this one is tricky! #[derive(Clone)] creates a T: Clone constraint implicitly, I think because Rust does not support perfect derives (due to some combination of action-at-a-distance hazard at the language design level, and trait-solver complication at the implementation level?). I found this out the hard way then had to add the manual derive below.

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:43):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:43):

cfallin created PR review comment:

(Addressed above -- the derived Clone doesn't work because of language restrictions)

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:45):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:45):

cfallin created PR review comment:

Updated description; thanks!

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:45):

cfallin created PR review comment:

Fixed!

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:45):

cfallin created PR review comment:

Fixed!

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:45):

cfallin created PR review comment:

No real reason, other than wanting to avoid excessive trims at very small root-set sizes; but you're right, better to trust the exponential growth and stick with a standard factor.

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:52):

cfallin commented on PR #11514:

Updating C API bits I think is fine, but naively I'm surprised that this is still using a generational index. I thought that was only necessary due to the manual nature of ManuallyRooted and for something with a dtor like this we wouldn't need generational indices? I haven't fully thought this through but that feels redundant now.

This is a plumbing issue, mostly, I think -- the GcRootIndex carries the generation (this is true because of the LIFO case still, at least, and the type is shared between LIFO and owned), and we need to reconstruct the GcRootIndex when the raw bits from C are passed back in. We could in theory read them out from the slab, but we would need access to the store to do that (ah, a familiar theme in my recent work!), so it seemed simpler not to touch the way it works other than to add the liveness flag. Happy to hear other thoughts if you or Nick have any (or see this refactored further on followup).

Considering the C API some more too, this'll want to go over documentation with a fine-tooth comb as well because we've probably documented "you should call unroot, but nothing bad happens if you don't so long as you free the store". Now that needs to be changed to "you must call unroot or host memory will leak". That's a pretty big departure in behavior from before, which while not bad we need to be sure is clearly communicated.

At least in wasmtime/val.h, we do have

* Anyref values are required to be explicitly unrooted via * #wasmtime_anyref_unroot to enable them to be garbage-collected.

I suppose that we don't warn more explicitly than that, but it seems reasonably documented that the usual "free what you allocate" C API rules apply? Happy to iterate here as well in followup!

Wasmtime GitHub notifications bot (Aug 25 2025 at 20:52):

cfallin has enabled auto merge for PR #11514.

Wasmtime GitHub notifications bot (Aug 25 2025 at 21:41):

alexcrichton commented on PR #11514:

As a minor request, could this use void* instead of usize (in C and Rust) to carry the "this is a pointer" bit along?

it seems reasonably documented that the usual "free what you allocate" C API rules apply?

My main worry about this is that it's the only allocated "value" in the C API (as a discriminant of wasmtime_valunion_t). All other values are scalars and were previously scalars too. I'm not sure how many users this would affect though in that I'm not sure anyone's using GC right now.

I know I've had historical issues binding owned values to other languages like Python and Go in the past, but I don't recall the specifics.

Wasmtime GitHub notifications bot (Aug 25 2025 at 23:34):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 25 2025 at 23:35):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 25 2025 at 23:37):

cfallin commented on PR #11514:

@alexcrichton sure thing!

See 20a929ba938a95ef4cee11ce9c6dc69932407969 for the pointer-rather-than-usize change; needs unsafe impls of Send/Sync on the wrapper struct in the C layer but this should be straightforwardly OK because it's an Arc (I think).

See 8b3bf8fb1d540cd3838632925ac1ba8901c22d2a for some additional warnings in the header docs about the need to unroot explicitly.

See the other commit loosening the size assert slightly because riscv32imac gotta be different wrt padding (sad).

Let me know what you think...

Wasmtime GitHub notifications bot (Aug 26 2025 at 14:17):

alexcrichton commented on PR #11514:

Oh, sorry, I'm just bemoaning the state of managed values. I don't really want to get this working with the C API in other languages but I'm not sure anyone else wants to either. I don't have a great alternative, however, so all I can point out is that it's objectively more dangerous to switch to a "real pointer" from an index because mistakes are segfaults rather than panics. That doesn't mean we shouldn't do this, so that's sort of me just bemoaning that there's not really any motivation to fully flesh this out on the C side right now

Wasmtime GitHub notifications bot (Aug 26 2025 at 16:08):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 26 2025 at 17:21):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 26 2025 at 17:26):

cfallin commented on PR #11514:

@fitzgen a review of one more bit if you don't mind -- I just pushed a07c3b0ef819db3956c3a0c2030dcda6a507bf5a that switches to a different amortized algorithm for root trimming.

It doesn't grow the high-water mark exponentially; instead it sets it adaptively based on the last post-trim live-root set size. I realized that the former will still result in unbounded liveness-flag-array growth between GCs if one creates and drops many GC roots without mutating the GC graph itself. The new algorithm is "self-equilibriating" -- it provides a strong guarantee that the size is at most double the max true live-set size, and still has amortized constant-time root creation. I wrote a proof in the doc-comment to convince myself :-)

Let me know what you think!

Wasmtime GitHub notifications bot (Aug 26 2025 at 17:32):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 26 2025 at 17:33):

fitzgen submitted PR review:

Ah yes of course. This is actually what I was assuming we were going to do in the original issue discussion and then it fled my mind when I actually reviewed the code. Looks great! And with a proof as well!

Wasmtime GitHub notifications bot (Aug 26 2025 at 17:36):

cfallin has enabled auto merge for PR #11514.

Wasmtime GitHub notifications bot (Aug 26 2025 at 17:56):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 26 2025 at 17:57):

cfallin commented on PR #11514:

(Added one more detail after staring at this again: should not have gated the high-water mark update on eager (GC) vs root-creation mode; only the early-out is gated on that.)

Wasmtime GitHub notifications bot (Aug 26 2025 at 17:57):

cfallin has enabled auto merge for PR #11514.

Wasmtime GitHub notifications bot (Aug 26 2025 at 18:34):

cfallin updated PR #11514.

Wasmtime GitHub notifications bot (Aug 26 2025 at 18:35):

cfallin has enabled auto merge for PR #11514.

Wasmtime GitHub notifications bot (Aug 26 2025 at 19:34):

cfallin merged PR #11514.

Last updated: Dec 06 2025 at 06:05 UTC