Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define garbage collection rooting APIs #8011

Merged
merged 25 commits into from
Mar 6, 2024

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Feb 28, 2024

Rooting prevents GC objects from being collected while they are actively being used.

We have a few sometimes-conflicting goals with our GC rooting APIs:

  1. Safety: It should never be possible to get a use-after-free bug because the user misused the rooting APIs, the collector "mistakenly" determined an object was unreachable and collected it, and then the user tried to access the object. This is our highest priority.

  2. Moving GC: Our rooting APIs should moving collectors (such as generational and compacting collectors) where an object might get relocated after a collection and we need to update the GC root's pointer to the moved object. This means we either need cooperation and internal mutability from individual GC roots as well as the ability to enumerate all GC roots on the native Rust stack, or we need a level of indirection.

  3. Performance: Our rooting APIs should generally be as low-overhead as possible. They definitely shouldn't require synchronization and locking to create, access, and drop GC roots.

  4. Ergonomics: Our rooting APIs should be, if not a pleasure, then at least not a burden for users. Additionally, the API's types should be Sync and Send so that they work well with async Rust.

For example, goals (3) and (4) are in conflict when we think about how to support (2). Ideally, for ergonomics, a root would automatically unroot itself when dropped. But in the general case that requires holding a reference to the store's root set, and that root set needs to be held simultaneously by all GC roots, and they each need to mutate the set to unroot themselves. That implies Rc<RefCell<...>> or Arc<Mutex<...>>! The former makes the store and GC root types not Send and not Sync. The latter imposes synchronization and locking overhead. So we instead make GC roots indirect and require passing in a store context explicitly to unroot in the general case. This trades worse ergonomics for better performance and support for moving GC and async Rust.

Okay, with that out of the way, this module provides two flavors of rooting API. One for the common, scoped lifetime case, and another for the rare case where we really need a GC root with an arbitrary, non-LIFO/non-scoped lifetime:

  1. RootScope and Rooted<T>: These are used for temporarily rooting GC objects for the duration of a scope. Upon exiting the scope, they are automatically unrooted. The internal implementation takes advantage of the LIFO property inherent in scopes, making creating and dropping Rooted<T>s and RootScopes super fast and roughly equivalent to bump allocation.

    This type is vaguely similar to V8's HandleScope.

    Note that Rooted<T> can't be statically tied to its context scope via a lifetime parameter, unfortunately, as that would allow the creation and use of only one Rooted<T> at a time, since the Rooted<T> would take a borrow of the whole context.

    This supports the common use case for rooting and provides good ergonomics.

  2. ManuallyRooted<T>: This is the fully general rooting API used for holding onto non-LIFO GC roots with arbitrary lifetimes. However, users must manually unroot them. Failure to manually unroot a ManuallyRooted<T> before it is dropped will result in the GC object (and everything it transitively references) leaking for the duration of the Store's lifetime.

    This type is roughly similar to SpiderMonkey's PersistentRooted<T>, although they avoid the manual-unrooting with internal mutation and shared references. (Our constraints mean we can't do those things, as mentioned explained above.)

At the end of the day, both Rooted<T> and ManuallyRooted<T> are just tagged indices into the store's RootSet. This indirection allows working with Rust's borrowing discipline (we use &mut Store to represent mutable access to the GC heap) while still allowing rooted references to be moved around without tying up the whole store in borrows. Additionally, and crucially, this indirection allows us to update the actual GC pointers in the RootSet and support moving GCs (again, as mentioned above).

@fitzgen fitzgen requested review from a team as code owners February 28, 2024 13:59
@fitzgen fitzgen requested review from elliottt and alexcrichton and removed request for a team February 28, 2024 13:59
@fitzgen
Copy link
Member Author

fitzgen commented Feb 28, 2024

Part of #5032

@fitzgen fitzgen force-pushed the rooting-api branch 5 times, most recently from e11a9c8 to a21e324 Compare February 28, 2024 15:04
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. wasmtime:ref-types Issues related to reference types and GC in Wasmtime labels Feb 28, 2024
Copy link

Subscribe to Label Action

cc @fitzgen, @peterhuene

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

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

  • fitzgen: fuzzing, wasmtime:ref-types
  • peterhuene: wasmtime:api, wasmtime:c-api

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

Learn more.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a high level this all looks good to me. I think there's work we can do to build confidence in the internals, though. While I realize we can't remove all unsafe here I suspect we don't need quite so many just-a-typed-transmute functions. Those are really difficult to reason about the safety.

Additionally I think this is definitely an area where we're going to be leaning on miri pretty heavily. Can you ensure that there's tests that run in miri doing all the various bits and bobs with the API other than actually calling in to wasm?

crates/runtime/src/externref/gc.rs Show resolved Hide resolved
crates/wasmtime/src/runtime/ref/gc_ref.rs Outdated Show resolved Hide resolved
tests/all/gc.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/func.rs Show resolved Hide resolved
}

#[cfg(feature = "gc")]
unsafe impl WasmTy for ManuallyRooted<ExternRef> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a future PR, and with more GC types, I think it'd be reasonable to move these impls to the gc_ref.rs module to avoid the #[cfg] here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea.

crates/wasmtime/src/runtime/ref/gc_ref/rooting.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/ref/gc_ref/rooting.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/ref/gc_ref/rooting.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/ref/gc_ref/rooting.rs Outdated Show resolved Hide resolved
crates/runtime/src/externref/gc.rs Outdated Show resolved Hide resolved
@fitzgen
Copy link
Member Author

fitzgen commented Feb 29, 2024

@alexcrichton I think I've addressed everything in your review.

Going to start rebasing on main, but I think you can take another look now.

Rooting prevents GC objects from being collected while they are actively being
used.

We have a few sometimes-conflicting goals with our GC rooting APIs:

1. Safety: It should never be possible to get a use-after-free bug because the
   user misused the rooting APIs, the collector "mistakenly" determined an
   object was unreachable and collected it, and then the user tried to access
   the object. This is our highest priority.

2. Moving GC: Our rooting APIs should moving collectors (such as generational
   and compacting collectors) where an object might get relocated after a
   collection and we need to update the GC root's pointer to the moved
   object. This means we either need cooperation and internal mutability from
   individual GC roots as well as the ability to enumerate all GC roots on the
   native Rust stack, or we need a level of indirection.

3. Performance: Our rooting APIs should generally be as low-overhead as
   possible. They definitely shouldn't require synchronization and locking to
   create, access, and drop GC roots.

4. Ergonomics: Our rooting APIs should be, if not a pleasure, then at least not
   a burden for users. Additionally, the API's types should be `Sync` and `Send`
   so that they work well with async Rust.

For example, goals (3) and (4) are in conflict when we think about how to
support (2). Ideally, for ergonomics, a root would automatically unroot itself
when dropped. But in the general case that requires holding a reference to the
store's root set, and that root set needs to be held simultaneously by all GC
roots, and they each need to mutate the set to unroot themselves. That implies
`Rc<RefCell<...>>` or `Arc<Mutex<...>>`! The former makes the store and GC root
types not `Send` and not `Sync`. The latter imposes synchronization and locking
overhead. So we instead make GC roots indirect and require passing in a store
context explicitly to unroot in the general case. This trades worse ergonomics
for better performance and support for moving GC and async Rust.

Okay, with that out of the way, this module provides two flavors of rooting
API. One for the common, scoped lifetime case, and another for the rare case
where we really need a GC root with an arbitrary, non-LIFO/non-scoped lifetime:

1. `RootScope` and `Rooted<T>`: These are used for temporarily rooting GC
   objects for the duration of a scope. Upon exiting the scope, they are
   automatically unrooted. The internal implementation takes advantage of the
   LIFO property inherent in scopes, making creating and dropping `Rooted<T>`s
   and `RootScope`s super fast and roughly equivalent to bump allocation.

   This type is vaguely similar to V8's [`HandleScope`].

   [`HandleScope`]: https://v8.github.io/api/head/classv8_1_1HandleScope.html

   Note that `Rooted<T>` can't be statically tied to its context scope via a
   lifetime parameter, unfortunately, as that would allow the creation and use
   of only one `Rooted<T>` at a time, since the `Rooted<T>` would take a borrow
   of the whole context.

   This supports the common use case for rooting and provides good ergonomics.

2. `ManuallyRooted<T>`: This is the fully general rooting API used for holding
   onto non-LIFO GC roots with arbitrary lifetimes. However, users must manually
   unroot them. Failure to manually unroot a `ManuallyRooted<T>` before it is
   dropped will result in the GC object (and everything it transitively
   references) leaking for the duration of the `Store`'s lifetime.

   This type is roughly similar to SpiderMonkey's [`PersistentRooted<T>`],
   although they avoid the manual-unrooting with internal mutation and shared
   references. (Our constraints mean we can't do those things, as mentioned
   explained above.)

   [`PersistentRooted<T>`]: http://devdoc.net/web/developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS::PersistentRooted.html

At the end of the day, both `Rooted<T>` and `ManuallyRooted<T>` are just tagged
indices into the store's `RootSet`. This indirection allows working with Rust's
borrowing discipline (we use `&mut Store` to represent mutable access to the GC
heap) while still allowing rooted references to be moved around without tying up
the whole store in borrows. Additionally, and crucially, this indirection allows
us to update the *actual* GC pointers in the `RootSet` and support moving GCs
(again, as mentioned above).
Remove some transmute methods, assert that `VMExternRef`s are the only valid
`VMGcRef`, etc.
Until we can add proper GC rooting.
@alexcrichton alexcrichton added this pull request to the merge queue Mar 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 5, 2024
@fitzgen fitzgen enabled auto-merge March 5, 2024 17:43
@fitzgen fitzgen added this pull request to the merge queue Mar 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 5, 2024
@fitzgen fitzgen enabled auto-merge March 5, 2024 23:05
@fitzgen fitzgen added this pull request to the merge queue Mar 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 5, 2024
@fitzgen fitzgen requested a review from a team as a code owner March 6, 2024 00:19
@fitzgen fitzgen enabled auto-merge March 6, 2024 00:19
@fitzgen fitzgen added this pull request to the merge queue Mar 6, 2024
Merged via the queue into bytecodealliance:main with commit bd2ea90 Mar 6, 2024
19 checks passed
@fitzgen fitzgen deleted the rooting-api branch March 6, 2024 01:08
@alexcrichton
Copy link
Member

Handling the fallout for this in the wasmtime-cpp repository I think that the requirement to take a wasmtime_context_t as an argument for value operations invalidates the preexisting copy constructor and copy assignment operator. Not an issue per se, but it appears that removing those also invalidates this pattern where arguments can't be constructed as {6, 27} any more. I'll admit I'm not C++ expert and the wall of error messages I'm looking at don't exactly explain why it's related to the copy constructor/assignment.

Anyway that's a long way of asking, do you think we're going to indefinitely want to have a context argument on these functios into the future, even with the idea of an indexed heap?

@fitzgen
Copy link
Member Author

fitzgen commented Mar 8, 2024

Anyway that's a long way of asking, do you think we're going to indefinitely want to have a context argument on these functios into the future, even with the idea of an indexed heap?

Yes, unfortunately, I do. At least in Rust. I could see a C/C++ specific approach to alleviating this, however...

Indexed GC heaps are orthogonal to the rooting approach here.

In order to support moving GCs, we need to either:

  1. Be able to precisely identify raw (whether pointer or index) GC roots and mutate them after an object moves.

  2. Or support pinning GC references temporarily such that they do not move while rooted. This allows us to avoid the need to update GC roots after an object moves.

Option (2) is a pretty big constraint on GC implementations. We would have to really bend over backwards to support that in our planned copying collector.

So focusing on option (1), there are basically two approaches available to us for implementing rooting APIs:

  1. Indirection. Store the raw GC references in some sort of table (aka our GcRoots) and have the user's root type index into that table (aka our Rooted<T>/ManuallyRooted<T>). This is nice, and works well with Rust and its move semantics, because we don't need to track down where all the Rooted<T>s and ManuallyRooted<T>s are in memory and we don't need to pin them or do any sort of updates on move so that we can update their internal GC reference after an object moves. Instead, the GC refs are all in the table and we can just do a pass over the table to update the GC refs, and all the Rooted<T>s index into the table and get the moved GC ref the next time they do some operation on their referent. The downside is that all constructing and dropping GC roots in this scheme requires access to the table, in the general case, as you've found.

  2. Intrusive lists/some other intrusive data structure. This is the approach that SpiderMonkey takes, for example. All the user's roots hold raw GC references and are additionally members of an intrusive list that is associated with their GC heap. This allows the GC to walk the intrusive list and update all rooted GC references after moving objects. But this means that you need to pin the GC root type or have move constructors that keep the intrusive list valid as the GC root is moved in memory. This works well with C++ but very much not so well with Rust, which is why we went with the first option.

So, if we wanted to make the C/C++ API a little more ergonomic, we could support the intrusive list option. Ideally not in the Rust embedder API, but maybe with some doc(hidden) stuff that is feature gated for when we are building the C API crate. FWIW, this is also fairly unsafe not just in terms of the invariants around maintianing the intrusive list, but in that right now with the table/indirect approach we only ever use indices and we bounds check their accesses into the table so "use after free"-style bugs are still memory safe, which limits how bad things can go wrong. That is not the case with the intrusive list approach.

@alexcrichton
Copy link
Member

Ok nah that all sounds good, no need to go the intrusive list route just yet, I think it's ok if things are slightly less ergonomic in the C++ bindings API unless a C++ wizard more adept than I can figure out a better solution

@rockwotj
Copy link
Contributor

Not an issue per se, but it appears that removing those also invalidates this pattern where arguments can't be constructed as {6, 27} any more

https://github.com/bytecodealliance/wasmtime-cpp/blob/da579e78f799aca0a472875b7e348f74b3a04145/include/wasmtime.hh#L2457C32-L2458

The function you're calling takes a std::vector and you're passing in an initializer_list, which copies things out of the vector: https://godbolt.org/z/h844YKxYq

You'll need to use a move iterator to force moving out of the container. One of the annoying bits about C++ iterators. You could use ranges/views to make this more seemless.

As for dropping the copy constructor/assignment I think that's fine, but it can be useful to have an explicit copy function that takes a context.

@alexcrichton
Copy link
Member

Makes sense! In bytecodealliance/wasmtime-cpp#48 I ended up adding overloaded versions for std::initializer_list and std::vector with a "base" version that takes an iterator. That being said I don't know how to best generically take an iterator in C++

@rockwotj
Copy link
Contributor

kpreisser added a commit to kpreisser/wasmtime-dotnet that referenced this pull request Jun 24, 2024
This includes updates for:
- bytecodealliance/wasmtime#8451
- bytecodealliance/wasmtime#8461
- bytecodealliance/wasmtime#8011

TODOs:
- Allocating an `externref` can now fail (by `wasmtime_externref_new` returning `false`). Currently, we throw a `WasmtimeException` in that case. We need to check where that exception can be thrown, and whether we need to do any additional clean-up (e.g. when converting arguments for a function call).
- Check whether it's ok to compare the `__private` field of externs (which has been remaned in the C API, previously it was `index`).
- `anyref` type is not yet supported, but I'm not sure what exactly it is and whether we need to add it.

Fixes bytecodealliance#315
jsturtevant pushed a commit to bytecodealliance/wasmtime-dotnet that referenced this pull request Jun 28, 2024
* Update to recent Wasmtime C API changes regarding values.

This includes updates for:
- bytecodealliance/wasmtime#8451
- bytecodealliance/wasmtime#8461
- bytecodealliance/wasmtime#8011

TODOs:
- Allocating an `externref` can now fail (by `wasmtime_externref_new` returning `false`). Currently, we throw a `WasmtimeException` in that case. We need to check where that exception can be thrown, and whether we need to do any additional clean-up (e.g. when converting arguments for a function call).
- Check whether it's ok to compare the `__private` field of externs (which has been remaned in the C API, previously it was `index`).
- `anyref` type is not yet supported, but I'm not sure what exactly it is and whether we need to add it.

Fixes #315

* Follow-Up: Make fields private.

* Ensure to clean-up `Value` instances (in arguments for a function call, and in results for an untyped callback) when e.g. allocating an `externref` fails.

We don't need to do such a clean-up for unchecked function calls that use `ValueRaw` because in that case we don't own `externref` values.

* Avoid accessing the `__private` fields in tests by checking the whole struct for equality (which is the case when all members are equal).

* Use separate dictionaries for caching `Function`, `Memory`, and `Global` objects in the `Store`, which avoids having to explicitly accessing the `__private` field (because the whole struct is now compared).

Additionally, it is more type-safe (since we don't need to cast the `object`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. wasmtime:ref-types Issues related to reference types and GC in Wasmtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants