Remove need for explicit Config::async_support knob #12371
Remove need for explicit Config::async_support knob #12371alexcrichton merged 4 commits intobytecodealliance:mainfrom
Config::async_support knob #12371Conversation
|
This is currently built on #12366, so draft for now. One other motivation for this I'm now remembering too -- I find it a bit odd that one of the more common ways of using Wasmtime, with async, requires explicit opt-in. This makes Wasmtime work "by default" in more situations and is a nice small win for that too. |
Subscribe to Label Actioncc @fitzgen DetailsThis issue or pull request has been labeled: "fuzzing", "wasi", "wasmtime:c-api"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
2c52129 to
de37ab0
Compare
fitzgen
left a comment
There was a problem hiding this comment.
Only looked at the second commit b/c I think the first is part of a different PR, right?
LGTM, just a nitpick and a question below.
|
|
||
| impl HostFunc { | ||
| fn new<T, F, P, R>(func: F) -> Arc<HostFunc> | ||
| fn new<T, F, P, R>(requires_async: bool, func: F) -> Arc<HostFunc> |
There was a problem hiding this comment.
Does this need to be an unsafe function now? What is the worst that can happen if a caller passes requires_async = false when F actually does need to be async? Will that result in safe panics on block_on/with_blocking?
There was a problem hiding this comment.
Good point yeah, I'll document that. Should be safe panicking though, so no new unsafe needed
| pub async fn collect_async<'a>( | ||
| mut collection: Box<dyn GarbageCollection<'a> + 'a>, | ||
| async_yield: bool, | ||
| asyncness: Asyncness, |
There was a problem hiding this comment.
Other functions in this commit have requires_async: bool -- can they switch to using Asyncness as well?
There was a problem hiding this comment.
Good point, I like that!
Push the `async`-ness down one layer.
This commit is an attempt to step towards reconciling "old async" and
"new async" in Wasmtime. The old async style is the original async
support in Wasmtime with `call_async`, `func_wrap_async`, etc, where the
main property is that the store is "locked" during an async operation.
Put another way, a store can only execute at most one async operation at
a time. This is in contrast to "new async" support in Wasmtime with the
component-model-async (WASIp3) support, where stores can have more than
one async operation in flight at once.
This commit does not fully reconcile these differences, but it does
remove one hurdle along the way: `Config::async_support`. Since the
beginning of Wasmtime this configuration knob has existed to explicitly
demarcate a config/engine/store as "this thing requires `async` stuff
internally." This has started to make less and less sense over time
where the line between sync and async has become more murky with WASIp3
where the two worlds comingle. The goal of this commit is to deprecate
`Config::async_support` and make the function not actually do anything.
In isolation this can't simply be done, however, because there are many
load-bearing aspects of Wasmtime that rely on this `async_support` knob.
For example once epochs + yielding are enabled it's required that all
Wasm is executed on a fiber lest it hit an epoch and not know how to
yield. That means that this commit is not a simple removal of
`async_support` but instead a refactoring/rearchitecting of how async is
used internally within Wasmtime. The high-level ideas within Wasmtime
now are:
* A `Store` has a "requires async" boolean stored within it.
* All configuration options which end up requiring async, such as
yielding with epochs, turn this boolean on.
* Creation of host functions which use async
(e.g. `func_wrap_{async,concurrent}`) will also turn this option on.
* Synchronous API entrypoints into Wasmtime ensure that this boolean is
disabled.
* Asynchronous APIs are usable at any time.
This means that the concept of an async store vs a sync store is now
gone. All stores are equally capable of executing sync/async, and the
change now is that dynamically some stores will require that async is
used with certain configuration. Additionally all panicking conditions
around `async_support` have been converted to errors instead. All
relevant APIs already returned an error and things are murky enough now
that it's not necessarily trivial to get this right at the embedder
level. In the interest of avoiding panics all detected async mismatches
are now first-class `wasmtime::Error` values.
The end result of this commit is that `Config::async_support` is a
deprecated `#[doc(hidden)]` function that does nothing. While many
internal changes happened as well as having new tests for all this sort
of behavior this is not expected to have a great impact on external
consumers. In general a deletion of `async_support(true)` is in theory
all that's required. This is intended to make it easier to think about
async/sync/etc in the future with WASIp3 and eventually reconcile
`func_wrap_async` and `func_wrap_concurrent` for example. That's left
for future refactorings however.
prtest:full
de37ab0 to
bc19c1f
Compare
This commit is an attempt to step towards reconciling "old async" and
"new async" in Wasmtime. The old async style is the original async
support in Wasmtime with
call_async,func_wrap_async, etc, where themain property is that the store is "locked" during an async operation.
Put another way, a store can only execute at most one async operation at
a time. This is in contrast to "new async" support in Wasmtime with the
component-model-async (WASIp3) support, where stores can have more than
one async operation in flight at once.
This commit does not fully reconcile these differences, but it does
remove one hurdle along the way:
Config::async_support. Since thebeginning of Wasmtime this configuration knob has existed to explicitly
demarcate a config/engine/store as "this thing requires
asyncstuffinternally." This has started to make less and less sense over time
where the line between sync and async has become more murky with WASIp3
where the two worlds comingle. The goal of this commit is to deprecate
Config::async_supportand make the function not actually do anything.In isolation this can't simply be done, however, because there are many
load-bearing aspects of Wasmtime that rely on this
async_supportknob.For example once epochs + yielding are enabled it's required that all
Wasm is executed on a fiber lest it hit an epoch and not know how to
yield. That means that this commit is not a simple removal of
async_supportbut instead a refactoring/rearchitecting of how async isused internally within Wasmtime. The high-level ideas within Wasmtime
now are:
Storehas a "requires async" boolean stored within it.yielding with epochs, turn this boolean on.
(e.g.
func_wrap_{async,concurrent}) will also turn this option on.disabled.
This means that the concept of an async store vs a sync store is now
gone. All stores are equally capable of executing sync/async, and the
change now is that dynamically some stores will require that async is
used with certain configuration. Additionally all panicking conditions
around
async_supporthave been converted to errors instead. Allrelevant APIs already returned an error and things are murky enough now
that it's not necessarily trivial to get this right at the embedder
level. In the interest of avoiding panics all detected async mismatches
are now first-class
wasmtime::Errorvalues.The end result of this commit is that
Config::async_supportis adeprecated
#[doc(hidden)]function that does nothing. While manyinternal changes happened as well as having new tests for all this sort
of behavior this is not expected to have a great impact on external
consumers. In general a deletion of
async_support(true)is in theoryall that's required. This is intended to make it easier to think about
async/sync/etc in the future with WASIp3 and eventually reconcile
func_wrap_asyncandfunc_wrap_concurrentfor example. That's leftfor future refactorings however.