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

Introduce wasmtime::StructRef and allocating Wasm GC structs #8933

Merged
merged 14 commits into from
Jul 11, 2024

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Jul 10, 2024

This commit introduces the wasmtime::StructRef type and support for allocating Wasm GC structs from the host. This commit does not add support for the struct.new family of Wasm instructions; guests still cannot allocate Wasm GC objects yet, but initial support should be pretty straightforward after this commit lands.

The StructRef type has everything you expect from other value types in the wasmtime crate:

  • A method to get its type or check whether it matches a given type

  • An implementation of WasmTy so that it can be used with Func::wrap-style APIs

  • The ability to upcast it into an AnyRef and to do checked downcasts in the opposite direction

There are, additionally, methods for getting, setting, and enumerating a StructRef's fields.

To allocate a StructRef, we need proof that the struct type we are allocating is being kept alive for the duration that the allocation may live. This is required for many reasons, but a basic example is getting a struct instance's type from the embedder API: this does a type-index-to-StructType lookup and conversion and if the type wasn't kept alive, then the type-index lookup will result in what is logically a use-after-free bug. This won't be a problem for Wasm guests (when we get around to implementing allocation for them) since their module defines the type, the store holds onto its instances' modules, and the allocation cannot outlive the store. For the host, we need another method of keeping the object's type alive, since it might be that the host defined the type and there is no module that also defined it, let alone such a module that is being kept alive in the store.

The solution to the struct-type-lifetime problem that this commit implements for hosts is for the store to hold a hash set of RegisteredTypes specifically for objects which were allocated via the embedder API. But we also don't want to do a hash lookup on every allocation, so we also implement a StructRefPre type. A StructRefPre is proof that the embedder has inserted a StructType's inner RegisteredType into a store. Structurally, it is a pair of the struct type and a store id. All StructRef allocation methods require a StructRefPre argument, which does a fast store id check, rather than a whole hash table insertion.

I opted to require StructRefPre in all allocation cases -- even though this has the downside of always forcing callers to create one before they allocate, even if they are only allocating a single object -- because of two reasons. First, this avoids needing to define duplicate methods, with and without a StructRefPre argument. Second, this avoids a performance footgun in the API where users don't realize that they can avoid extra work by creating a single StructRefPre and then using it multiple times. Anecdotally, I've heard multiple people complain about instantiation being slower than advertised but it turns out they weren't using InstancePre, and I'd like to avoid that situation for allocation if we can.

This commit introduces the `wasmtime::StructRef` type and support for allocating
Wasm GC structs from the host. This commit does *not* add support for the
`struct.new` family of Wasm instructions; guests still cannot allocate Wasm GC
objects yet, but initial support should be pretty straightforward after this
commit lands.

The `StructRef` type has everything you expect from other value types in the
`wasmtime` crate:

* A method to get its type or check whether it matches a given type

* An implementation of `WasmTy` so that it can be used with `Func::wrap`-style
  APIs

* The ability to upcast it into an `AnyRef` and to do checked downcasts in the
  opposite direction

There are, additionally, methods for getting, setting, and enumerating a
`StructRef`'s fields.

To allocate a `StructRef`, we need proof that the struct type we are allocating
is being kept alive for the duration that the allocation may live. This is
required for many reasons, but a basic example is getting a struct instance's
type from the embedder API: this does a type-index-to-`StructType` lookup and
conversion and if the type wasn't kept alive, then the type-index lookup will
result in what is logically a use-after-free bug. This won't be a problem for
Wasm guests (when we get around to implementing allocation for them) since their
module defines the type, the store holds onto its instances' modules, and the
allocation cannot outlive the store. For the host, we need another method of
keeping the object's type alive, since it might be that the host defined the
type and there is no module that also defined it, let alone such a module that
is being kept alive in the store.

The solution to the struct-type-lifetime problem that this commit implements for
hosts is for the store to hold a hash set of `RegisteredType`s specifically for
objects which were allocated via the embedder API. But we also don't want to do
a hash lookup on every allocation, so we also implement a `StructRefPre` type. A
`StructRefPre` is proof that the embedder has inserted a `StructType`'s inner
`RegisteredType` into a store. Structurally, it is a pair of the struct type and
a store id. All `StructRef` allocation methods require a `StructRefPre`
argument, which does a fast store id check, rather than a whole hash table
insertion.

I opted to require `StructRefPre` in all allocation cases -- even though this
has the downside of always forcing callers to create one before they allocate,
even if they are only allocating a single object -- because of two
reasons. First, this avoids needing to define duplicate methods, with and
without a `StructRefPre` argument. Second, this avoids a performance footgun in
the API where users don't realize that they *can* avoid extra work by creating a
single `StructRefPre` and then using it multiple times. Anecdotally, I've heard
multiple people complain about instantiation being slower than advertised but it
turns out they weren't using `InstancePre`, and I'd like to avoid that situation
for allocation if we can.
@fitzgen fitzgen requested a review from a team as a code owner July 10, 2024 20:13
@fitzgen fitzgen requested review from alexcrichton and removed request for a team July 10, 2024 20:13
Comment on lines +320 to +321
// TODO: `dec_ref_and_maybe_dealloc` each `VMGcRef` inside this
// object.
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this is intentionally left for follow up PRs. I want to focus on getting the spec tests passing and all that first, especially since the DRC collector is just the stand in until we eventually grow our semi-space collector.

@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:ref-types Issues related to reference types and GC in Wasmtime labels Jul 10, 2024
Copy link

Subscribe to Label Action

cc @fitzgen

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.

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.

I'm liking how this is all shaping up, very nice! I've got cosmetic comments here and there but nothing major.

crates/wasmtime/src/runtime/gc/disabled/structref.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/gc/enabled/rooting.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/gc/enabled/structref.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/values.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/vm/gc/enabled/structref.rs Outdated Show resolved Hide resolved
@fitzgen fitzgen enabled auto-merge July 11, 2024 20:37
@fitzgen fitzgen added this pull request to the merge queue Jul 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 11, 2024
@fitzgen fitzgen requested a review from a team as a code owner July 11, 2024 21:23
@fitzgen fitzgen requested review from cfallin and removed request for a team July 11, 2024 21:23
@fitzgen fitzgen enabled auto-merge July 11, 2024 21:23
@fitzgen fitzgen added this pull request to the merge queue Jul 11, 2024
Merged via the queue into bytecodealliance:main with commit f2e689c Jul 11, 2024
120 checks passed
@fitzgen fitzgen deleted the host-alloc-gc-objects branch July 11, 2024 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself 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.

2 participants