yield to executor when polling with zero-duration wait#13085
yield to executor when polling with zero-duration wait#13085dicej merged 3 commits intobytecodealliance:mainfrom
Conversation
| async fn yield_to_executor(config: &Config) { | ||
| // TODO: Once `Config` has an optional `AsyncFn` field for yielding to the | ||
| // current async runtime (e.g. `tokio::task::yield_now`), use that if set; | ||
| // otherwise fall back to the runtime-agnostic code below. | ||
| _ = config; | ||
|
|
||
| let mut yielded = false; | ||
| future::poll_fn(move |cx| { | ||
| if yielded { | ||
| Poll::Ready(()) | ||
| } else { | ||
| yielded = true; | ||
| cx.waker().wake_by_ref(); | ||
| Poll::Pending | ||
| } | ||
| }) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
Could this be moved to a method on StoreOpaque? For example:
impl StoreOpaque {
async fn yield_now(&mut self) {
// ...
}
}There was a problem hiding this comment.
Also, could you double check we're not doing this sort of yield elsewhere in Wasmtime? I feel like we probably do this in at least one more location that can be deduplicated with this ideally.
There was a problem hiding this comment.
There was identical code in runtime/vm/async_yield.rs, which I've consolidated into the new StoreOpaque::yield_now function. There's one awkward case, though: StoreOpaque::do_gc uses unwrap_gc_store_mut to get exclusive access to the GcStore, which means we can't use the containing StoreOpaque during the call to GcStore::gc. For now, I'm just passing a freestanding function to GcStore::gc for it to use when it needs to yield. Once we have e.g. a Config::yield_fn field of type Option<fn() -> Box<dyn Future<Output = ()> + Send + Sync>>, we'll be able to pass in a closure which closes over that.
5fdf95d to
59b5cc7
Compare
This addresses bytecodealliance#13040 for `wasmtime-wasi`'s WASIp2 implementation by calling `tokio::task::yield_now` in `<Deadline as Pollable>::ready` when `self` is a `Deadline::Past`. Previously, it did nothing, which had the effect of starving other pollables that rely on yielding control to `mio` in order to progress, e.g. when the guest polls with a zero timeout in a busy loop. This also addresses that issue for `wasmtime`'s `thread.yield` implementation by modifying the `poll_until` event loop in `concurrent.rs` to yield to the executor prior to executing any low-priority tasks. This works because `thread.yield` operates by queueing a low-priority task to resume the calling fiber just prior to suspending it. Note that, because the `wasmtime` crate does not depend on Tokio, we can't directly call `tokio::task::yield_now`. Instead, we return a `Future` which, when polled for the first time, wakes the `Context`'s `Waker` and returns `Poll::Pending`; then it returns `Poll::Ready(())` for subsequent polls. That's enough to ensure `mio` has a chance to run when using `tokio`. Once we have a mechanism to allow the embedder to configure a custom yield function, we'll be able to use that instead. Fixes bytecodealliance#13040
Now that zero-duration `monotonic_clock` waits yield to the executor, they aren't ready immediately, so the assertions in this test are no longer appropriate.
- Expand comments regarding why we yield to the runtime in `wasmtime` and `wasmtime-wasi` - Move `yield_to_executor` to `StoreOpaque::yield_now` and remove `vm/async_yield` in favor of the new function - Revert `take_low_priority` performance tweak (will open a separate PR for that) - Inline `collect_work_itmes_to_run` for clarity
This addresses #13040 for
wasmtime-wasi's WASIp2 implementation by callingtokio::task::yield_nowin<Deadline as Pollable>::readywhenselfis aDeadline::Past. Previously, it did nothing, which had the effect of starving other pollables that rely on yielding control tomioin order to progress, e.g. when the guest polls with a zero timeout in a busy loop.This also addresses that issue for
wasmtime'sthread.yieldimplementation by modifying thepoll_untilevent loop inconcurrent.rsto yield to the executor prior to executing any low-priority tasks. This works becausethread.yieldoperates by queueing a low-priority task to resume the calling fiber just prior to suspending it.Note that, because the
wasmtimecrate does not depend on Tokio, we can't directly calltokio::task::yield_now. Instead, we return aFuturewhich, when polled for the first time, wakes theContext'sWakerand returnsPoll::Pending; then it returnsPoll::Ready(())for subsequent polls. That's enough to ensuremiohas a chance to run when usingtokio. Once we have a mechanism to allow the embedder to configure a custom yield function, we'll be able to use that instead.Fixes #13040