-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Make const-expr evaluation async
#11468
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
Make const-expr evaluation async
#11468
Conversation
This commit is extracted from bytecodealliance#11430 to accurately reflect how const-expr evaluation is an async operation due to GC pauses that may happen. The changes in this commit are: * Const-expr evaluation is, at its core, now an `async` function. * To leverage this new `async`-ness all internal operations are switched from `*_maybe_async` to `*_async` meaning all the `*_maybe_async` methods can be removed. * Some libcalls using `*_maybe_async` are switch to using `*_async` plus the `block_on!` utility to help jettison more `*_maybe_async` methods. * Instance initialization is now an `async` function. This is temporarily handled with `block_on` during instance initialization to avoid propagating the `async`-ness further upwards. This `block_on` will get deleted in future refactorings. * Const-expr evaluation has been refactored slightly to enable having a fast path in global initialization which skips an `await` point entirely, achieving performance-parity in benchmarks prior to this commit. This ended up fixing a niche issue with GC where if a wasm execution was suspended during `table.init`, for example, during a const-expr evaluation triggering a GC then if the wasm execution was cancelled it would panic the host. This panic was because the GC operation returned `Result` but it was `unwrap`'d as part of the const-expr evaluation which can fail not only to invalid-ness but also due to "computation is cancelled" traps.
|
Actually, thinking more, the issue about panicking on const expr evaluation failure means allocation failure in GC is a host panic which isn't great. Deferring that to a separate issue. |
Subscribe to Label Actioncc @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:
To subscribe or unsubscribe from this label, edit the |
fitzgen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| }; | ||
| block_on!(store, async |store| { | ||
| let gc_ref = store | ||
| .retry_after_gc_async((), |store, ()| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any non-async retry_after_gc calls at this point? I think all of them should be async by the time we finish the asyncification of the internals, but I'm not sure if we are there yet or not. But when we are, we should delete the non-async version and then just rename the async variation to plain retry_after_gc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Soon! (got a PR queued up after #11470 to delete the sync version
| if store.async_support() { | ||
| #[cfg(feature = "async")] | ||
| store.block_on(|store| { | ||
| let module = compiled_module.module().clone(); | ||
| Box::pin( | ||
| async move { vm::initialize_instance(store, id, &module, bulk_memory).await }, | ||
| ) | ||
| })??; | ||
| #[cfg(not(feature = "async"))] | ||
| unreachable!(); | ||
| } else { | ||
| vm::assert_ready(vm::initialize_instance( | ||
| store, | ||
| id, | ||
| compiled_module.module(), | ||
| bulk_memory, | ||
| ))?; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then Instance::new_raw will become an async function in another follow up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed! That'll be #11470
* Make const-expr evaluation `async` This commit is extracted from bytecodealliance#11430 to accurately reflect how const-expr evaluation is an async operation due to GC pauses that may happen. The changes in this commit are: * Const-expr evaluation is, at its core, now an `async` function. * To leverage this new `async`-ness all internal operations are switched from `*_maybe_async` to `*_async` meaning all the `*_maybe_async` methods can be removed. * Some libcalls using `*_maybe_async` are switch to using `*_async` plus the `block_on!` utility to help jettison more `*_maybe_async` methods. * Instance initialization is now an `async` function. This is temporarily handled with `block_on` during instance initialization to avoid propagating the `async`-ness further upwards. This `block_on` will get deleted in future refactorings. * Const-expr evaluation has been refactored slightly to enable having a fast path in global initialization which skips an `await` point entirely, achieving performance-parity in benchmarks prior to this commit. This ended up fixing a niche issue with GC where if a wasm execution was suspended during `table.init`, for example, during a const-expr evaluation triggering a GC then if the wasm execution was cancelled it would panic the host. This panic was because the GC operation returned `Result` but it was `unwrap`'d as part of the const-expr evaluation which can fail not only to invalid-ness but also due to "computation is cancelled" traps. * Fix configured build * Undo rebase mistake
This commit is extracted from #11430 to accurately reflect how const-expr evaluation is an async operation due to GC pauses that may happen. The changes in this commit are:
asyncfunction.async-ness all internal operations are switched from*_maybe_asyncto*_asyncmeaning all the*_maybe_asyncmethods can be removed.*_maybe_asyncare switch to using*_asyncplus theblock_on!utility to help jettison more*_maybe_asyncmethods.asyncfunction. This is temporarily handled withblock_onduring instance initialization to avoid propagating theasync-ness further upwards. Thisblock_onwill get deleted in future refactorings.awaitpoint entirely, achieving performance-parity in benchmarks prior to this commit.This ended up fixing a niche issue with GC where if a wasm execution was suspended during
table.init, for example, during a const-expr evaluation triggering a GC then if the wasm execution was cancelled it would panic the host. This panic was because the GC operation returnedResultbut it wasunwrap'd as part of the const-expr evaluation which can fail not only to invalid-ness but also due to "computation is cancelled" traps.