Skip to content

Conversation

@alexcrichton
Copy link
Member

Upon further refactoring and thinking about #11430 I've realized that we might be able to sidestep T: Send on the store entirely which would be quite the boon if it can be pulled off. The realization I had is that the main reason for this was &mut dyn VMStore on the stack, but that itself is actually a bug in Wasmtime (#11178) and shouldn't be done. The functions which have this on the stack should actually ONLY have the resource limiter, if configured. This means that while the ResourceLimiter{,Async} traits need a Send supertrait that's relatively easy to add without much impact. My hunch is that plumbing this through to the end will enable all the benefits of #11430 without requiring adding T: Send to the store.

This commit starts out on this journey by making table growth a true async fn. A new internal type is added to represent a store's limiter which is plumbed to growth functions. This represents a hierarchy of borrows that look like:

  • StoreInner<T>
    • StoreResourceLimiter<'_>
    • StoreOpaque
      • Pin<&mut Instance>
        • &mut vm::Table

This notably, safely, allows operating on vm::Table with a StoreResourceLimiter at the same time. This is exactly what's needed and prevents needing to have &mut dyn VMStore, the previous argument, on the stack.

This refactoring cleans up unsafe blocks in table growth right now which manually uses raw pointers to work around the borrow checker. No more now!

I'll note as well that this is just an incremental step. What I plan on doing next is handling other locations like memory growth, memory allocation, and table allocation. Each of those will require further refactorings to ensure that things like GC are correctly accounted for so they're going to be split into separate PRs. Functionally though this PR should have no impact other than a fiber is no longer required for Table::grow_async.

Upon further refactoring and thinking about bytecodealliance#11430 I've realized that we
might be able to sidestep `T: Send` on the store entirely which would be
quite the boon if it can be pulled off. The realization I had is that
the main reason for this was `&mut dyn VMStore` on the stack, but that
itself is actually a bug in Wasmtime (bytecodealliance#11178) and shouldn't be done.
The functions which have this on the stack should actually ONLY have the
resource limiter, if configured. This means that while the
`ResourceLimiter{,Async}` traits need a `Send` supertrait that's
relatively easy to add without much impact. My hunch is that plumbing
this through to the end will enable all the benefits of bytecodealliance#11430 without
requiring adding `T: Send` to the store.

This commit starts out on this journey by making table growth a true
`async fn`. A new internal type is added to represent a store's limiter
which is plumbed to growth functions. This represents a hierarchy of
borrows that look like:

* `StoreInner<T>`
  * `StoreResourceLimiter<'_>`
  * `StoreOpaque`
    * `Pin<&mut Instance>`
      * `&mut vm::Table`

This notably, safely, allows operating on `vm::Table` with a
`StoreResourceLimiter` at the same time. This is exactly what's needed
and prevents needing to have `&mut dyn VMStore`, the previous argument,
on the stack.

This refactoring cleans up `unsafe` blocks in table growth right
now which manually uses raw pointers to work around the borrow checker.
No more now!

I'll note as well that this is just an incremental step. What I plan on
doing next is handling other locations like memory growth, memory
allocation, and table allocation. Each of those will require further
refactorings to ensure that things like GC are correctly accounted for
so they're going to be split into separate PRs. Functionally though this
PR should have no impact other than a fiber is no longer required for
`Table::grow_async`.
@alexcrichton alexcrichton requested a review from a team as a code owner August 15, 2025 21:46
@alexcrichton alexcrichton requested review from pchickey and removed request for a team August 15, 2025 21:46
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Aug 16, 2025
@alexcrichton alexcrichton added this pull request to the merge queue Aug 18, 2025
Merged via the queue into bytecodealliance:main with commit 5f7cf53 Aug 18, 2025
44 checks passed
@alexcrichton alexcrichton deleted the really-start-the-async-journey branch August 18, 2025 18:23
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Aug 19, 2025
This is an analog of bytecodealliance#11442 but for memories. This had a little more
impact due to memories being hooked into GC operations. Further
refactoring of GC operations to make them safer/more-async is deferred
to a future PR and for now it's "no worse than before". This is another
step towards bytecodealliance#11430 and enables removing a longstanding `unsafe` block
in `runtime/memory.rs` which previously could not be removed.

One semantic change from this is that growth of a shared memory no
longer uses an async limiter. This is done to keep growth of a shared
memory consistent with creation of a shared memory where no limits are
applied. This is due to the cross-store nature of shared memories which
means that we can't tie growth to any one particular store. This
additionally fixes an issue where an rwlock write guard was otherwise
held across a `.await` point which creates a non-`Send` future, closing
a possible soundness hole in Wasmtime.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Aug 19, 2025
This is an analog of bytecodealliance#11442 but for memories. This had a little more
impact due to memories being hooked into GC operations. Further
refactoring of GC operations to make them safer/more-async is deferred
to a future PR and for now it's "no worse than before". This is another
step towards bytecodealliance#11430 and enables removing a longstanding `unsafe` block
in `runtime/memory.rs` which previously could not be removed.

One semantic change from this is that growth of a shared memory no
longer uses an async limiter. This is done to keep growth of a shared
memory consistent with creation of a shared memory where no limits are
applied. This is due to the cross-store nature of shared memories which
means that we can't tie growth to any one particular store. This
additionally fixes an issue where an rwlock write guard was otherwise
held across a `.await` point which creates a non-`Send` future, closing
a possible soundness hole in Wasmtime.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Aug 19, 2025
This commit is similar to bytecodealliance#11442 and bytecodealliance#11460 except it's applied to the
garbage collection phase of Wasmtime's GC. Specifically the functions
that actually perform a GC are no longer duplicated across sync, async,
and maybe async versions. There's only one "always async" version and
the root-level crate entrypoints for sync versions assert that async
support is disabled and then use the async version.

Worth noting here is that GC suffers from a preexisting issue described
in bytecodealliance#11409 where it's not sound how a `StoreOpaque` is widened to acquire
a resource limiter. This commit seemingly makes the issue worse by
adding a few more `unsafe` blocks, but they're all fundamentally doing
the same thing as before. Fully solving this issue will require making
memory/table creation an `async` function that takes the limiter as an
argument. Doing this will require further refactoring/code movement so
my goal is to effectively maintain the status quo, but in a slightly
different location, and enable knocking out the `unsafe` in the future.
In the meantime the previous `unsafe` block is "lifted higher up" so
it's not quite so deep and should be easier to remove in the future.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Aug 19, 2025
This is an analog of bytecodealliance#11442 but for memories. This had a little more
impact due to memories being hooked into GC operations. Further
refactoring of GC operations to make them safer/more-async is deferred
to a future PR and for now it's "no worse than before". This is another
step towards bytecodealliance#11430 and enables removing a longstanding `unsafe` block
in `runtime/memory.rs` which previously could not be removed.

One semantic change from this is that growth of a shared memory no
longer uses an async limiter. This is done to keep growth of a shared
memory consistent with creation of a shared memory where no limits are
applied. This is due to the cross-store nature of shared memories which
means that we can't tie growth to any one particular store. This
additionally fixes an issue where an rwlock write guard was otherwise
held across a `.await` point which creates a non-`Send` future, closing
a possible soundness hole in Wasmtime.
github-merge-queue bot pushed a commit that referenced this pull request Aug 19, 2025
* Make memory growth an `async` function

This is an analog of #11442 but for memories. This had a little more
impact due to memories being hooked into GC operations. Further
refactoring of GC operations to make them safer/more-async is deferred
to a future PR and for now it's "no worse than before". This is another
step towards #11430 and enables removing a longstanding `unsafe` block
in `runtime/memory.rs` which previously could not be removed.

One semantic change from this is that growth of a shared memory no
longer uses an async limiter. This is done to keep growth of a shared
memory consistent with creation of a shared memory where no limits are
applied. This is due to the cross-store nature of shared memories which
means that we can't tie growth to any one particular store. This
additionally fixes an issue where an rwlock write guard was otherwise
held across a `.await` point which creates a non-`Send` future, closing
a possible soundness hole in Wasmtime.

* Fix threads-disabled build

* Review comments
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Aug 19, 2025
This commit is similar to bytecodealliance#11442 and bytecodealliance#11460 except it's applied to the
garbage collection phase of Wasmtime's GC. Specifically the functions
that actually perform a GC are no longer duplicated across sync, async,
and maybe async versions. There's only one "always async" version and
the root-level crate entrypoints for sync versions assert that async
support is disabled and then use the async version.

Worth noting here is that GC suffers from a preexisting issue described
in bytecodealliance#11409 where it's not sound how a `StoreOpaque` is widened to acquire
a resource limiter. This commit seemingly makes the issue worse by
adding a few more `unsafe` blocks, but they're all fundamentally doing
the same thing as before. Fully solving this issue will require making
memory/table creation an `async` function that takes the limiter as an
argument. Doing this will require further refactoring/code movement so
my goal is to effectively maintain the status quo, but in a slightly
different location, and enable knocking out the `unsafe` in the future.
In the meantime the previous `unsafe` block is "lifted higher up" so
it's not quite so deep and should be easier to remove in the future.
github-merge-queue bot pushed a commit that referenced this pull request Aug 19, 2025
* Make garbage collection an `async` function

This commit is similar to #11442 and #11460 except it's applied to the
garbage collection phase of Wasmtime's GC. Specifically the functions
that actually perform a GC are no longer duplicated across sync, async,
and maybe async versions. There's only one "always async" version and
the root-level crate entrypoints for sync versions assert that async
support is disabled and then use the async version.

Worth noting here is that GC suffers from a preexisting issue described
in #11409 where it's not sound how a `StoreOpaque` is widened to acquire
a resource limiter. This commit seemingly makes the issue worse by
adding a few more `unsafe` blocks, but they're all fundamentally doing
the same thing as before. Fully solving this issue will require making
memory/table creation an `async` function that takes the limiter as an
argument. Doing this will require further refactoring/code movement so
my goal is to effectively maintain the status quo, but in a slightly
different location, and enable knocking out the `unsafe` in the future.
In the meantime the previous `unsafe` block is "lifted higher up" so
it's not quite so deep and should be easier to remove in the future.

* Review comments

* Fix configured build

* More configured build fixes
bongjunj pushed a commit to prosyslab/wasmtime that referenced this pull request Oct 20, 2025
* Make table growth a true `async fn`

Upon further refactoring and thinking about bytecodealliance#11430 I've realized that we
might be able to sidestep `T: Send` on the store entirely which would be
quite the boon if it can be pulled off. The realization I had is that
the main reason for this was `&mut dyn VMStore` on the stack, but that
itself is actually a bug in Wasmtime (bytecodealliance#11178) and shouldn't be done.
The functions which have this on the stack should actually ONLY have the
resource limiter, if configured. This means that while the
`ResourceLimiter{,Async}` traits need a `Send` supertrait that's
relatively easy to add without much impact. My hunch is that plumbing
this through to the end will enable all the benefits of bytecodealliance#11430 without
requiring adding `T: Send` to the store.

This commit starts out on this journey by making table growth a true
`async fn`. A new internal type is added to represent a store's limiter
which is plumbed to growth functions. This represents a hierarchy of
borrows that look like:

* `StoreInner<T>`
  * `StoreResourceLimiter<'_>`
  * `StoreOpaque`
    * `Pin<&mut Instance>`
      * `&mut vm::Table`

This notably, safely, allows operating on `vm::Table` with a
`StoreResourceLimiter` at the same time. This is exactly what's needed
and prevents needing to have `&mut dyn VMStore`, the previous argument,
on the stack.

This refactoring cleans up `unsafe` blocks in table growth right
now which manually uses raw pointers to work around the borrow checker.
No more now!

I'll note as well that this is just an incremental step. What I plan on
doing next is handling other locations like memory growth, memory
allocation, and table allocation. Each of those will require further
refactorings to ensure that things like GC are correctly accounted for
so they're going to be split into separate PRs. Functionally though this
PR should have no impact other than a fiber is no longer required for
`Table::grow_async`.

* Remove #[cfg] gate
bongjunj pushed a commit to prosyslab/wasmtime that referenced this pull request Oct 20, 2025
* Make memory growth an `async` function

This is an analog of bytecodealliance#11442 but for memories. This had a little more
impact due to memories being hooked into GC operations. Further
refactoring of GC operations to make them safer/more-async is deferred
to a future PR and for now it's "no worse than before". This is another
step towards bytecodealliance#11430 and enables removing a longstanding `unsafe` block
in `runtime/memory.rs` which previously could not be removed.

One semantic change from this is that growth of a shared memory no
longer uses an async limiter. This is done to keep growth of a shared
memory consistent with creation of a shared memory where no limits are
applied. This is due to the cross-store nature of shared memories which
means that we can't tie growth to any one particular store. This
additionally fixes an issue where an rwlock write guard was otherwise
held across a `.await` point which creates a non-`Send` future, closing
a possible soundness hole in Wasmtime.

* Fix threads-disabled build

* Review comments
bongjunj pushed a commit to prosyslab/wasmtime that referenced this pull request Oct 20, 2025
* Make garbage collection an `async` function

This commit is similar to bytecodealliance#11442 and bytecodealliance#11460 except it's applied to the
garbage collection phase of Wasmtime's GC. Specifically the functions
that actually perform a GC are no longer duplicated across sync, async,
and maybe async versions. There's only one "always async" version and
the root-level crate entrypoints for sync versions assert that async
support is disabled and then use the async version.

Worth noting here is that GC suffers from a preexisting issue described
in bytecodealliance#11409 where it's not sound how a `StoreOpaque` is widened to acquire
a resource limiter. This commit seemingly makes the issue worse by
adding a few more `unsafe` blocks, but they're all fundamentally doing
the same thing as before. Fully solving this issue will require making
memory/table creation an `async` function that takes the limiter as an
argument. Doing this will require further refactoring/code movement so
my goal is to effectively maintain the status quo, but in a slightly
different location, and enable knocking out the `unsafe` in the future.
In the meantime the previous `unsafe` block is "lifted higher up" so
it's not quite so deep and should be easier to remove in the future.

* Review comments

* Fix configured build

* More configured build fixes
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants