GC: replace ManuallyRooted with OwnedRooted.

[cfallin]

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.

[github-actions]

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 .github/subscribe-to-label.json configuration file.

Learn more.

[cfallin]

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)

[alexcrichton]

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.

[fitzgen]

Fantastic! Thanks for whipping this up!

[cfallin]

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!

[alexcrichton]

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.

[cfallin]

@alexcrichton sure thing!

• See 20a929b 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 8b3bf8f 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...

[alexcrichton]

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

[cfallin]

@fitzgen a review of one more bit if you don't mind -- I just pushed a07c3b0 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!

[fitzgen]

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!

[cfallin]

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