-
Notifications
You must be signed in to change notification settings - Fork 1.6k
generalize async fiber abstraction #11114
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
generalize async fiber abstraction #11114
Conversation
As part of the work implementing the new Component Model async ABI in the `wasip3-prototyping` repo, I've generalized the `FiberFuture` abstraction in `wasmtime::runtime::store::async_` to support fibers which can either retain exclusive access to the store across suspend points or release it. The latter allows the store to be used by the `component-model-async` event loop and/or other fibers to run before the original fiber resumes, which is the key to allowing multiple fibers to run concurrently, passing control of the store back and forth. In the case of Pulley, the above generalization means we also need to give each fiber its own `Interpreter` so that multiple concurrent fibers don't clobber each other's state. Concretely, this moves a lot of the code out of `async_.rs` and into a new `fiber.rs` submodule which will be shared with the `component-model-async` implementation. This also pulls in a new `StoreToken<T>` utility which has been useful in `wasip3-prototyping` to safely convert from a `&mut dyn VMStore` to a `StoreContextMut<'a, T>` when we previously witnessed a conversion in the other direction. Note that I've added a `'static` bound to the `VMStore` trait, which simplifies use of `&mut dyn VMStore`, avoiding thorny lifetime issues. Signed-off-by: Joel Dice <[email protected]>
5ce9d98 to
07bd52f
Compare
This pulls in the latest Component Model async ABI code from the `wasip3-prototyping` repo, including various API refactors and spec updates. This includes all the changes to the `wasmtime` crate from `wasip3-prototyping` _except_ that the `concurrent` submodule and child submodules contain only non-functional stubs. For that reason, and the fact that e.g. `Func::call_async` is now implemented in terms of `Func::call_concurrent`, most of the component model tests are failing. This commit is not meant to be merged as-is; a follow-up commit (to be PR'd separately) will contain the real `concurrent` implementation, at which point the tests will pass again. I'm splitting these into separate PRs to make review easier. Note that this builds on bytecodealliance#11114; only the second commit is new. Signed-off-by: Joel Dice <[email protected]>
alexcrichton
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.
I'll keep taking a look at fiber.rs, but this is at least an initial round of feedback.
This commit replaces the stub functions and types in `wasmtime::runtime::component::concurrent` and its submodules with the working implementation developed in the `wasip3-prototyping` repo. For ease of review, it does not include any new tests; I'll add those in a follow-up commit. Note that this builds on bytecodealliance#11114 and bytecodealliance#11123; only the third commit is new. Signed-off-by: Joel Dice <[email protected]>
This commit replaces the stub functions and types in `wasmtime::runtime::component::concurrent` and its submodules with the working implementation developed in the `wasip3-prototyping` repo. For ease of review, it does not include any new tests; I'll add those in a follow-up commit. Note that this builds on bytecodealliance#11114 and bytecodealliance#11123; only the third commit is new. Signed-off-by: Joel Dice <[email protected]>
Signed-off-by: Joel Dice <[email protected]>
As part of my earlier effort to unify the fiber abstractions in the `wasmtime` crate, I changed a `*mut StoreFiber` field to a `&mut StoreFiber`, not realizing that it resulted in a mutable alias at runtime and thus undefined behavior. Miri caught it, fortunately. Signed-off-by: Joel Dice <[email protected]>
This commit replaces the stub functions and types in `wasmtime::runtime::component::concurrent` and its submodules with the working implementation developed in the `wasip3-prototyping` repo. For ease of review, it does not include any new tests; I'll add those in a follow-up commit. Note that this builds on bytecodealliance#11114 and bytecodealliance#11123; only the third commit is new. Signed-off-by: Joel Dice <[email protected]>
Signed-off-by: Joel Dice <[email protected]>
Main changes: - Make `resume_fiber[_raw]` take a `&mut StoreOpaque` parameter to make its unsafe internals easier to reason about, safety-wise. - Panic if `StoreFiber::drop` is called on an in-progress fiber without having called `StoreFiber::dispose` to gracefully end it first. - (Re)introduce `FiberFuture`, which closes over a `&mut StoreOpaque` and uses it to call `StoreFiber::dispose` on drop. This will require a few more changes to make it usable by `concurrent.rs`, but I'll save those changes for a later PR. Signed-off-by: Joel Dice <[email protected]>
Signed-off-by: Joel Dice <[email protected]>
Signed-off-by: Joel Dice <[email protected]>
May be needed for concurrent bits, but for now not necessary.
It's predicted Miri won't like this, but for now in-repo it's ok with it.
Remove the need for the function entirely and replace it with `resume_fiber`.
Can use stack-based closures/results to transmit the result instead of needing a channel.
The `on_fiber` function is small enough it should be possible to do so.
Leave the fiber non-optional at-rest so it's always available for the destructor.
Small shims, not otherwise public at this time, so remove a layer of indirection.
Helps remove some raw pointers that are held for a long time within `AsyncCx`
Signed-off-by: Joel Dice <[email protected]>
This commit replaces the stub functions and types in `wasmtime::runtime::component::concurrent` and its submodules with the working implementation developed in the `wasip3-prototyping` repo. For ease of review, it does not include any new tests; I'll add those in a follow-up commit. Note that this builds on bytecodealliance#11114 and bytecodealliance#11123; only the third commit is new. Signed-off-by: Joel Dice <[email protected]>
This pulls in the tests from the `wasip3-prototyping` repo, minus the ones requiring WASIp3 support in `wasmtime-wasi[-http]`, which will be PR'd separately. Note that this builds on bytecodealliance#11114, bytecodealliance#11123, and bytecodealliance#11127; only the fourth commit is new. Signed-off-by: Joel Dice <[email protected]>
Signed-off-by: Joel Dice <[email protected]>
Signed-off-by: Joel Dice <[email protected]>
Move management of the async guard range elsewhere to the normal save/restore area.
* Remove the `AsyncCx` type from Wasmtime as it's inherently `unsafe` to use, instead bundle operations directly on a `Store*` reference. * Don't retain pointers-to-pointers within the roughly-equivalent `BlockingContext` created in this PR. Instead when a blocking context is created "take" the metadata from the store to assert exclusive ownership of the pointers. * Refactor how `&mut Context<'_>` is passed around, namely thread it through fiber parameters to model resumption as registering a new context to poll with. * Remove `PollContext` in favor of directly storing a pointer as it's now mostly an empty structure. * Minor refactorings to make things more future-refactorable and/or clear in a few places. * Refactor management of the "current suspend" and "current future context" pointers. These are now null'd out on resumption and asserted null on suspension. * Remove the need for a generic `Reset` structure in the fiber bits as it's a pretty dangerous structure to have in general. The end result of this refactoring is that all usage of `block_on` is now safe and additionally many of the internals of the implementation are safer than they were before
This pulls in the latest Component Model async ABI code from the `wasip3-prototyping` repo, including various API refactors and spec updates. This includes all the changes to the `wasmtime` crate from `wasip3-prototyping` _except_ that the `concurrent` submodule and child submodules contain only non-functional stubs. For that reason, and the fact that e.g. `Func::call_async` is now implemented in terms of `Func::call_concurrent`, most of the component model tests are failing. This commit is not meant to be merged as-is; a follow-up commit (to be PR'd separately) will contain the real `concurrent` implementation, at which point the tests will pass again. I'm splitting these into separate PRs to make review easier. Note that this builds on bytecodealliance#11114; only the second commit is new. Signed-off-by: Joel Dice <[email protected]>
This commit replaces the stub functions and types in `wasmtime::runtime::component::concurrent` and its submodules with the working implementation developed in the `wasip3-prototyping` repo. For ease of review, it does not include any new tests; I'll add those in a follow-up commit. Note that this builds on bytecodealliance#11114 and bytecodealliance#11123; only the third commit is new. Signed-off-by: Joel Dice <[email protected]>
This pulls in the tests from the `wasip3-prototyping` repo, minus the ones requiring WASIp3 support in `wasmtime-wasi[-http]`, which will be PR'd separately. Note that this builds on bytecodealliance#11114, bytecodealliance#11123, and bytecodealliance#11127; only the fourth commit is new. Signed-off-by: Joel Dice <[email protected]>
No need for raw pointers with recent refactorings.
alexcrichton
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.
@dicej ok these are all the changes I'd like to make, mind reviewing the changes I made as well before merging? Afterwards feel free to go ahead and queue this for merge
Signed-off-by: Joel Dice <[email protected]>
Signed-off-by: Joel Dice <[email protected]>
* generalize async fiber abstraction As part of the work implementing the new Component Model async ABI in the `wasip3-prototyping` repo, I've generalized the `FiberFuture` abstraction in `wasmtime::runtime::store::async_` to support fibers which can either retain exclusive access to the store across suspend points or release it. The latter allows the store to be used by the `component-model-async` event loop and/or other fibers to run before the original fiber resumes, which is the key to allowing multiple fibers to run concurrently, passing control of the store back and forth. In the case of Pulley, the above generalization means we also need to give each fiber its own `Interpreter` so that multiple concurrent fibers don't clobber each other's state. Concretely, this moves a lot of the code out of `async_.rs` and into a new `fiber.rs` submodule which will be shared with the `component-model-async` implementation. This also pulls in a new `StoreToken<T>` utility which has been useful in `wasip3-prototyping` to safely convert from a `&mut dyn VMStore` to a `StoreContextMut<'a, T>` when we previously witnessed a conversion in the other direction. Note that I've added a `'static` bound to the `VMStore` trait, which simplifies use of `&mut dyn VMStore`, avoiding thorny lifetime issues. Signed-off-by: Joel Dice <[email protected]> * address review feedback Signed-off-by: Joel Dice <[email protected]> * fix miri-flagged stacked borrow violation As part of my earlier effort to unify the fiber abstractions in the `wasmtime` crate, I changed a `*mut StoreFiber` field to a `&mut StoreFiber`, not realizing that it resulted in a mutable alias at runtime and thus undefined behavior. Miri caught it, fortunately. Signed-off-by: Joel Dice <[email protected]> * remove unneeded `Send` bounds Signed-off-by: Joel Dice <[email protected]> * address more review feedback Main changes: - Make `resume_fiber[_raw]` take a `&mut StoreOpaque` parameter to make its unsafe internals easier to reason about, safety-wise. - Panic if `StoreFiber::drop` is called on an in-progress fiber without having called `StoreFiber::dispose` to gracefully end it first. - (Re)introduce `FiberFuture`, which closes over a `&mut StoreOpaque` and uses it to call `StoreFiber::dispose` on drop. This will require a few more changes to make it usable by `concurrent.rs`, but I'll save those changes for a later PR. Signed-off-by: Joel Dice <[email protected]> * address more review feedback Signed-off-by: Joel Dice <[email protected]> * update `impl Send For StoreFiber` comment Signed-off-by: Joel Dice <[email protected]> * Remove currently-extraneous `Result<()>` from fibers May be needed for concurrent bits, but for now not necessary. * Use safe pointers instead of raw pointers It's predicted Miri won't like this, but for now in-repo it's ok with it. * Fold more responsibility into `resume_fiber_raw` Remove the need for the function entirely and replace it with `resume_fiber`. * Remove channels from async fibers Can use stack-based closures/results to transmit the result instead of needing a channel. * Fold `on_fiber_raw` directly into `on_fiber` The `on_fiber` function is small enough it should be possible to do so. * Don't use `Option` in `FiberFuture` Leave the fiber non-optional at-rest so it's always available for the destructor. * Fold `suspend` functions together Small shims, not otherwise public at this time, so remove a layer of indirection. * Move stack limit management to `FiberResumeState` Helps remove some raw pointers that are held for a long time within `AsyncCx` * add some doc comments to `fiber.rs` Signed-off-by: Joel Dice <[email protected]> * update `fiber.rs` and friends to match CM async requirements This adds a `resolve_or_release` function, which `Instance::resume_fiber` will use when current `concurrent.rs` stub is replaced by a real implementation. Signed-off-by: Joel Dice <[email protected]> * fix non-component-model-async build warnings Signed-off-by: Joel Dice <[email protected]> * make `resume_fiber` private in `fiber.rs` Signed-off-by: Joel Dice <[email protected]> * Shrink `PollContext` state Move management of the async guard range elsewhere to the normal save/restore area. * Refactor `AsyncCx`, reduce `unsafe` * Remove the `AsyncCx` type from Wasmtime as it's inherently `unsafe` to use, instead bundle operations directly on a `Store*` reference. * Don't retain pointers-to-pointers within the roughly-equivalent `BlockingContext` created in this PR. Instead when a blocking context is created "take" the metadata from the store to assert exclusive ownership of the pointers. * Refactor how `&mut Context<'_>` is passed around, namely thread it through fiber parameters to model resumption as registering a new context to poll with. * Remove `PollContext` in favor of directly storing a pointer as it's now mostly an empty structure. * Minor refactorings to make things more future-refactorable and/or clear in a few places. * Refactor management of the "current suspend" and "current future context" pointers. These are now null'd out on resumption and asserted null on suspension. * Remove the need for a generic `Reset` structure in the fiber bits as it's a pretty dangerous structure to have in general. The end result of this refactoring is that all usage of `block_on` is now safe and additionally many of the internals of the implementation are safer than they were before * Adjust some lint attributes * Make manipulation of `AsyncState` safe No need for raw pointers with recent refactorings. * Fix dead code warning * More dead code warnings * Cut down on raw pointers in fiber.rs * Move executor save/restore to normal fiber state save/restore * Bikeshed a method name * update comment in make_fiber Signed-off-by: Joel Dice <[email protected]> * fix machports build Signed-off-by: Joel Dice <[email protected]> --------- Signed-off-by: Joel Dice <[email protected]> Co-authored-by: Alex Crichton <[email protected]>
As part of the work implementing the new Component Model async ABI in the
wasip3-prototypingrepo, I've generalized theFiberFutureabstraction inwasmtime::runtime::store::async_to support fibers which can either retain exclusive access to the store across suspend points or release it. The latter allows the store to be used by thecomponent-model-asyncevent loop and/or other fibers to run before the original fiber resumes, which is the key to allowing multiple fibers to run concurrently, passing control of the store back and forth.In the case of Pulley, the above generalization means we also need to give each fiber its own
Interpreterso that multiple concurrent fibers don't clobber each other's state.Concretely, this moves a lot of the code out of
async_.rsand into a newfiber.rssubmodule which will be shared with thecomponent-model-asyncimplementation.This also pulls in a new
StoreToken<T>utility which has been useful inwasip3-prototypingto safely convert from a&mut dyn VMStoreto aStoreContextMut<'a, T>when we previously witnessed a conversion in the other direction.Note that I've added a
'staticbound to theVMStoretrait, which simplifies use of&mut dyn VMStore, avoiding thorny lifetime issues.