| b4475438 | 30-Jul-2025 |
Joel Dice <[email protected]> |
refactor `{Stream,Future}|{Reader,Writer}` APIs and internals (#11325)
* refactor `{Stream,Future}|{Reader,Writer}` APIs and internals
This makes a several changes to how `{Stream,Future}|{Reader,W
refactor `{Stream,Future}|{Reader,Writer}` APIs and internals (#11325)
* refactor `{Stream,Future}|{Reader,Writer}` APIs and internals
This makes a several changes to how `{Stream,Future}|{Reader,Writer}` work to
make them more efficient and, in some ways, more ergonomic:
- The background tasks have been removed, allowing reads and writes to complete without task context switching. We now only allocate and use oneshot channels lazily when the other end is not yet ready; this improves real world performance benchmarks (e.g. wasi-http request handling) considerably.
- Instances of `{Stream,Future}Reader` can now be lifted and lowered directly; no need for `Host{Stream,Future}` anymore.
- The type parameter for `Stream{Reader,Writer}` no longer refers to the buffer type -- just the payload type (i.e. `StreamReader<u8>` instead of `StreamReader<Vec<u8>>`), meaning any buffer type may be used for a given read or write operation. This also means the compiler needs help with type inference less often when calling `Instance::stream`.
- Instances of `{Stream,Future}|{Reader,Writer}` now require access to the store in order to be disposed of properly. I've added RAII wrapper structs (`WithAccessor[AndValue]`) to help with this, and also updated `Store::drop` and `Instance::run_concurrent` to ensure the store thread-local is set when dropping futures closing over `&Accessor`s.
- In order to ensure that resources containing `{Stream,Future}|{Reader,Writer}` instances are disposed of properly, I've added `LinkerInstance::resource_concurrent` and have updated `wasmtime-wit-bindgen` to use it. This gives resource drop functions access to a `StoreContextMut` via an `Accessor`, allowing the stream and future handles to be disposed of.
- In order to make this work, I had to change `Accessor::instance` from a `Instance` to an `Option<Instance>`, which is awkward but temporary since we're planning to remove `Accessor::instance` entirely once we've moved concurrent state from `ComponentInstance` to `Store`.
That problem of disposal is definitely the most awkward part of all this. In
simple cases, it's easy enough to ensure that read and write handles are
disposed of properly, but both `wasmtime-wasi` and `wasmtime-wasi-http` have
some pretty complicated functions where handles are passed between tasks and/or
stored inside resources, so it can be tricky to ensure proper disposal on all
code paths. I'm open to ideas for improving this, but I suspect we'll need new
Rust language features (e.g. linear types) to make it truly ergonomic, robust,
and efficient.
While testing the above, I discovered an issue with `Instance::poll_until` such
that it would prematurely give up and return a "deadlock" trap error, believing
that there was no further work to do, even though the future passed to it was
ready to resolve the next time it was polled. I've fixed this by polling it one
last time and only trapping if it returns pending.
Note that I've moved a few associated functions from `ConcurrentState` to
`Instance` (e.g. `guest_drop_writable` and others) since they now need access to
the store; they're unchanged otherwise. Apologies for the diff noise.
Finally, I've tweaked how `wasmtime serve` to poll the guest for content before
handing the response to Hyper, which helps performance by ensuring the first
content chunk can be sent with the same TCP packet as the beginning of the
response.
Signed-off-by: Joel Dice <[email protected]>
fix wasi p3 build and test failures
Signed-off-by: Joel Dice <[email protected]>
use `ManuallyDrop` instead of `Option` in `Dropper`
This allows us to drop its `value` field in-place, i.e. without moving it,
thereby upholding the `Pin` guarantee.
Signed-off-by: Joel Dice <[email protected]>
address review comments
- Remove `DropWithStoreAndValue` and friends; go back to taking a `fn() -> T` parameter in `Instance::future` instead
- Make `DropWithStore::drop[_with]` take `&mut self` instead of `self`
- Make `WithAccessor` and `DropWithStore` private
- Instead, I've added public `Guarded{Stream,Future}{Reader,Writer}` types for RAII
- and also `{Stream,Future}{Reader,Writer}::close[_with]` methods
- Use RAII in `FutureReader::read` and `FutureWriter::write` to ensure handles are dropped if the `Future` is dropped
Signed-off-by: Joel Dice <[email protected]>
* lower host stream/future writes in background task
This avoids unsoundness due to guest realloc calls while there are host embedder
frames on the stack.
Signed-off-by: Joel Dice <[email protected]>
* fix `tcp.rs` regressions
Signed-off-by: Joel Dice <[email protected]>
---------
Signed-off-by: Joel Dice <[email protected]>
show more ...
|
| 64bc3bd9 | 15-Jul-2025 |
Alex Crichton <[email protected]> |
Start to use `&Accessor<T, D>` more in concurrent code (#11238)
* Start to use `&Accessor<T, D>` more in concurrent code
After discussion with Joel we've concluded that while `&mut Accessor<T, D>`
Start to use `&Accessor<T, D>` more in concurrent code (#11238)
* Start to use `&Accessor<T, D>` more in concurrent code
After discussion with Joel we've concluded that while `&mut Accessor<T, D>`
was originally added to model host functions it is also appropriate to
use it to model embedder-rooted invocations of items such as wasm as
well. Effectively the conclusion we reached was that
`*::call_concurrent` should be taking `&Accessor`, not
`StoreContextMut`. This has a number of benefits to it over the previous
iteration:
* This makes exports behave more like imports where `Accessor` means
"you're in the concurrent world".
* This makes exports have an `async fn` signature which is easier to
read and understand.
* This automatically enforces the guarantee that the returned future is
only polled within the main event loop because the future is always
considered to close over the `&Accessor` provided meaning it
statically cannot live outside of the event loop.
* This paves the way forward to future refactorings to avoid storing so
much state within a `Store<T>` and instead try to store state directly
in futures themselves. This should make cancellation more natural and
eventually also remove `'static` bounds on params/results. Furthermore
this should make it easier to avoid spawning tasks internally by
storing state in futures instead of spawned tasks.
In doing this one of the main questions we were faced with was what to
do about `&mut Accessor<T, D>`, namely the `mut` part. With a mutable
accessor it would be only possible to call one function concurrently.
One option considered was to add combinators like `Accessor::join` and
`Accessor::race` but in the end we decided to avoid going that direction
and instead switch to `&Accessor<T, D>` everywhere, freely enabling
aliasing of the accessor. This has the downside that `Accessor::with` is
now a relatively dangerous function in that it can panic, but idiomatic
usage of it is not expected to panic as the distinction between the
`async` and sync boundary of `Accessor` vs `StoreContextMut` is expected
to naturally make the recursive panic condition of `with` rare to come
up in practice.
Concrete changes in this commit are:
* `Accessor::with` now requires `&self`.
* `Accessor::spawn` now requires `&self`.
* Host functions are now given `&Accessor`, not `&mut Accessor`.
* `{Typed,}Func::call_concurrent` is now an `async fn` which takes an
`&Accessor` instead of `StoreContextMut`.
* Guest bindings generation for concurrent invocations now looks exactly
like async bindings generation except for replacing `StoreContextMut`
with `Accessor`.
Note that this commit does not yet update the internal implementations
of these functions to benefit from the new abilities that taking
`&Accessor` implies. Instead that's deferred to a future update as
necessary. Instead this is only updating the public API of the
`wasmtime` crate to enable these refactorings in the future.
Also note that this does not yet update all functions to take
`&Accessor`. Notably futures and streams still need to be updated.
cc #11224
* Review comments
---------
Co-authored-by: Joel Dice <[email protected]>
show more ...
|