Stream: git-wasmtime

Topic: wasmtime / PR #11470 Make table/memory creation async fu...

Wasmtime GitHub notifications bot (Aug 19 2025 at 23:52):

alexcrichton opened PR #11470 from alexcrichton:table-memory-creation-async to bytecodealliance:main:

This commit is a large-ish refactor which is made possible by the many
previous refactorings to internals w.r.t. async-in-Wasmtime. The end
goal of this change is that table and memory allocation are both async
functions. Achieving this, however, required some refactoring to enable
it to work:

• To work with Send neither function can close over dyn VMStore.
  This required changing their Option<&mut dyn VMStore> arugment to
  Option<&mut StoreResourceLimiter<'_>>

• Somehow a StoreResourceLimiter needed to be acquired from an
  InstanceAllocationRequest. Previously the store was stored here as
  an unsafe raw pointer, but I've refactored this now so
  InstanceAllocationRequest directly stores &StoreOpaque and
  Option<&mut StoreResourceLimiter> meaning it's trivial to acquire
  them. This additionally means no more unsafe access of the store
  during instance allocation (yay!).

• Now-redundant fields of InstanceAllocationRequest were removed since
  they can be safely inferred from &StoreOpaque. For example passing
  around &Tunables is now all gone.

• Methods upwards from table/memory allocation to the
  InstanceAllocator trait needed to be made async. This includes new
  #[async_trait] methods for example.

• StoreOpaque::ensure_gc_store is now an async function. This
  internally carries a new unsafe block carried over from before with
  the raw point passed around in InstanceAllocationRequest. A future
  PR will delete this unsafe block, it's just temporary.

I attempted a few times to split this PR up into separate commits but
everything is relatively intertwined here so this is the smallest
"atomic" unit I could manage to land these changes and refactorings.

Wasmtime GitHub notifications bot (Aug 19 2025 at 23:52):

alexcrichton requested dicej for a review on PR #11470.

Wasmtime GitHub notifications bot (Aug 19 2025 at 23:52):

alexcrichton requested wasmtime-core-reviewers for a review on PR #11470.

Wasmtime GitHub notifications bot (Aug 19 2025 at 23:53):

alexcrichton commented on PR #11470:

I'll note that I've split this into two commits, the first of which is https://github.com/bytecodealliance/wasmtime/pull/11456 resurrected here. That commit cannot be split out to a second PR due to all the various constraints in play unfortunately, so this is a bit larger than I would have otherwise anticipated.

Wasmtime GitHub notifications bot (Aug 19 2025 at 23:55):

alexcrichton commented on PR #11470:

Oh, I should also note, this is a 5% performance penalty overhead to instance instantiation. Benchmarking makes me think it's primarily related to the Box-cost of #[async_trait]. We can burn down that cost I think if we really need from allocation-per-table-and-memory to once-per-module (aka two for most modules to one), but I don't know how to get away from it entirely. I subjectively concluded a few extra allocations is fine, but others can reasonably differ.

Wasmtime GitHub notifications bot (Aug 19 2025 at 23:56):

alexcrichton edited PR #11470:

This commit is a large-ish refactor which is made possible by the many
previous refactorings to internals w.r.t. async-in-Wasmtime. The end
goal of this change is that table and memory allocation are both async
functions. Achieving this, however, required some refactoring to enable
it to work:

• To work with Send neither function can close over dyn VMStore.
  This required changing their Option<&mut dyn VMStore> arugment to
  Option<&mut StoreResourceLimiter<'_>>

• Somehow a StoreResourceLimiter needed to be acquired from an
  InstanceAllocationRequest. Previously the store was stored here as
  an unsafe raw pointer, but I've refactored this now so
  InstanceAllocationRequest directly stores &StoreOpaque and
  Option<&mut StoreResourceLimiter> meaning it's trivial to acquire
  them. This additionally means no more unsafe access of the store
  during instance allocation (yay!).

• Now-redundant fields of InstanceAllocationRequest were removed since
  they can be safely inferred from &StoreOpaque. For example passing
  around &Tunables is now all gone.

• Methods upwards from table/memory allocation to the
  InstanceAllocator trait needed to be made async. This includes new
  #[async_trait] methods for example.

• StoreOpaque::ensure_gc_store is now an async function. This
  internally carries a new unsafe block carried over from before with
  the raw point passed around in InstanceAllocationRequest. A future
  PR will delete this unsafe block, it's just temporary.

I attempted a few times to split this PR up into separate commits but
everything is relatively intertwined here so this is the smallest
"atomic" unit I could manage to land these changes and refactorings.

cc #11430

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

alexcrichton updated PR #11470.

Wasmtime GitHub notifications bot (Aug 20 2025 at 00:53):

alexcrichton commented on PR #11470:

I was a bit afraid of this, but tests here will fail until I rebase this on top of #11468. So ready for review, but won't pass tests until #11468 lands first.

Wasmtime GitHub notifications bot (Aug 20 2025 at 17:54):

alexcrichton updated PR #11470.

Wasmtime GitHub notifications bot (Aug 20 2025 at 17:55):

alexcrichton requested fitzgen for a review on PR #11470.

Wasmtime GitHub notifications bot (Aug 20 2025 at 18:39):

alexcrichton updated PR #11470.

Wasmtime GitHub notifications bot (Aug 20 2025 at 18:42):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Aug 20 2025 at 18:42):

fitzgen created PR review comment:

And similarly assert that the config is async here?

Wasmtime GitHub notifications bot (Aug 20 2025 at 18:42):

fitzgen created PR review comment:

Should we not assert that this is a non-async config before the .expect(..) to catch mismatches a little earlier?

Wasmtime GitHub notifications bot (Aug 20 2025 at 18:42):

fitzgen created PR review comment:

Add a note for this to our items to discuss with the lang team, if we don't have one already?

Wasmtime GitHub notifications bot (Aug 20 2025 at 18:42):

fitzgen created PR review comment:

This is so nice to clean up

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

alexcrichton submitted PR review.

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

alexcrichton created PR review comment:

Oh this one's technically already fixed and just waiting on stabilization, and browsing https://github.com/rust-lang/rust/issues/110338 shows this is pretty well-known, so not a major issue.

Wasmtime GitHub notifications bot (Aug 20 2025 at 18:53):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 20 2025 at 18:53):

alexcrichton created PR review comment:

Oh this is actually replicating the documented behavior where this works with async_support so long as an async resource limiter isn't used. In that sense to avoid breaking the semantics here one_poll is what's required as opposed to an assert!(!async_support) plus vm::assert_ready.

Wasmtime GitHub notifications bot (Aug 20 2025 at 18:54):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 20 2025 at 18:54):

alexcrichton created PR review comment:

As I've been making these changes I've actually been undoing a lot of assert!(async_support)-style assertions. Previously that was required because on_fiber was immediately used which required async_support to be turned on, but now it's just normal Rust async functions so there's no reason to prevent usage when async_support is disabled. In that sense it's intentional that the assert here is lost, but how's that sound to you?

Wasmtime GitHub notifications bot (Aug 20 2025 at 18:55):

alexcrichton updated PR #11470.

Wasmtime GitHub notifications bot (Aug 21 2025 at 00:02):

alexcrichton commented on PR #11470:

I'm going to go ahead and land this, but @fitzgen if you have follow up comments I'm happy to address them.

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

alexcrichton merged PR #11470.

Wasmtime GitHub notifications bot (Aug 21 2025 at 16:51):

fitzgen commented on PR #11470:

I'm going to go ahead and land this, but @fitzgen if you have follow up comments I'm happy to address them.

Nope, looks good

Last updated: Dec 06 2025 at 07:03 UTC