Wasmtime GitHub notifications bot (Jan 19 2022 at 01:36): Wasmtime GitHub notifications bot (Jan 19 2022 at 01:36):cfallin opened PR #3699 from epoch-interruption to main:
(Builds on #3698)
This PR introduces a new way of performing cooperative timeslicing that
is intended to replace the "fuel" mechanism. The tradeoff is that this
mechanism interrupts with less precision: not at deterministic points
where fuel runs out, but rather when the Engine enters a new epoch. The
generated code instrumentation is substantially faster, however, because
it does not need to do as much work as when tracking fuel; it only loads
the global "epoch counter" and does a compare-and-branch at backedges
and function prologues.This change has been measured as ~twice as fast as fuel-based
timeslicing for some workloads, especially control-flow-intensive
workloads such as the SpiderMonkey JS interpreter on Wasm/WASI.The intended interface is that the embedder of the
Engineperforms an
engine.increment_epoch()call periodically, e.g. once per millisecond.
An async invocation of a Wasm guest on aStorecan specify a number of
epoch-ticks that are allowed before an async yield back to the
executor's event loop. (The initial amount and automatic "refills" are
configured on theStore, just as for fuel.) This call does only
signal-safe work (it increments anAtomicU64) so could be invoked from
a periodic signal, or from a thread that wakes up once per period.
Wasmtime GitHub notifications bot (Jan 19 2022 at 01:36):cfallin requested alexcrichton for a review on PR #3699.
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
I don't think that the result here can be ignored and the result should be plumbed back the wasmtime-runtime, the meaning of the
Trapis that, during the yield, the future was dropped so this wasm module needs to be cleaned up ASAP which basically means raising a trap to get back to the base of the stack.
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
I think it might be good to have a configuration mode where, like for fuel, an epoch also has the ability to raise a trap, basically as a form of interruption. This would enable us later to remove the current interruption mechanism which is subsequently redundant.
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
Could this comment how the deadline is store-local and not operated on by other threads so the
unsafehere should be ok? Also that this value is cached by compiled code on the stack but it's reloaded before any new epoch is entered.
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
There's a
#[doc(cfg(...))]incantation which is used elsewhere in Wasmtime which makes thte requirement for this feature show up in the documentation, could that be used here as well?
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
Ideally we'll have one location for documenting epochs, what they're used for, and perhaps an example or two as well. I think that this
Configmethod may be one of the better places to put the user-facing documentation. Ideally we'd have some sort of book chapter on all the methods of interrupting executing WebAssembly but for now I thinkConfigmethods will suffice. Could this get filled in with more high-level details and get an example?(or really as long as the "canonical documentation" for epochs is in one place and everywhere else refers to that I think it's ok. I'm not sure this is something we've otherwise been diligent or great about with fuel or other various mechanisms implemented by Wasmtime)
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
I think it'd be good to add a few more tests for epochs such as:
- Infinite loops can be interrupted from another thread
- Function entries can be interrupted from another thread (I think fuel has this test?)
- Something which exercises the double-check block, such as a function which calls another which changes the epoch but only one yield happens instead of multiple as it returns back up the stack or otherwise hits other functions.
- Dropping an in-progress future which is yielded on an epoch doesn't execute any further wasm.
- Initially a store's epoch deadline is zero and a yield immediately happens
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
Could this link to the
Configdocumentation for epochs?
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
Could the documentation here elaborate on how the delta provided is added to the current engine-configured epoch and that's the new deadline?
Also could this link to the
Configdocumentation for epochs?
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
The naming here and with
refill_epoch_deadlineis a little confusing to me, perhaps something like:
set_epoch_yield_delta- for this functionset_epoch- the below functionand along the lines of a different comment if
set_epochis called (which should be available without theasyncfeature I think) andset_epoch_yield_deltaisn't called then we can trap on the first epoch change)
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
Could this use
$foofor names of functions?
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
Can this be passed as an
Optioninstead of 0-or-nonzero?Also can the documentation for this function be updated to indicate that might be used for non-fuel-based-scenarios? (perhaps renaming the function as well)
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
One idea perhaps is for
new_epochto return the new epoch so a reload doesn't have to happen afterwards to cut down on the code size slightly.
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
Would you be able to experiment with saving this value into an
ir::Variableto avoid having to reload it?
Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):alexcrichton created PR review comment:
Would you be able to perform a quick check to get a gut feeling on the compiled code size overhead of epoch checks vs fuel checks?
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:07):cfallin updated PR #3699 from epoch-interruption to main.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:07):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:07):cfallin created PR review comment:
Fixed, thanks! This now has the trap-or-refill configurable behavior that fuel does as well.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:07):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:07):cfallin created PR review comment:
Nice, had no idea about this -- added!
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:08):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:08):cfallin created PR review comment:
Added!
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:08):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:08):cfallin created PR review comment:
Added a comment describing the safety here.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:09):cfallin updated PR #3699 from epoch-interruption to main.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:11):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:11):cfallin created PR review comment:
I've renamed the methods to
set_epoch_deadlinefor the "set deadline to N ticks in the future" action (analogous toadd_fuel), and addedepoch_deadline_trapandepoch_deadline_async_yield_and_updateto configure the two different behaviors when deadline is reached.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:11):cfallin created PR review comment:
Updated docs here and elsewhere!
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:11):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:12):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:12):cfallin created PR review comment:
I wrote a long doc comment here and linked it from all the other method doc-comments (and expanded the others significantly too). Hopefully this is a bit more polished now.
No example yet but I can add that too if we think it's important -- I see that fuel has some toplevel examples in
examples/; maybe that would be appropriate?
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:52):cfallin updated PR #3699 from epoch-interruption to main.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:52):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:52):cfallin created PR review comment:
Fixed -- this is now
async_yield_impland the fuel update happens inout_of_gas(one of its callers) instead.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:53):cfallin created PR review comment:
Added all of these -- thanks!
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:53):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:53):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:53):cfallin created PR review comment:
Done!
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:54):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:54):cfallin created PR review comment:
I went ahead and did this; I think it's better to let the regalloc keep it in a register when possible and spill as it needs, because the spill isn't any worse than eagerly reloading each time as we do now. I can test this in more detail tomorrow though.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:59):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 02:59):cfallin created PR review comment:
Just tested with a
spidermonkey.wasmI had laying around: baseline (no fuel or epoch-interruption) produced a 54MB.cwasmviawasmtime compile; with--consume-fuelI get 68MB; with--epoch-interruptionI get 62MB.So on that wasm module, fuel instrumentation has a 26% overhead while epoch instrumentation has a 15% overhead.
Wasmtime GitHub notifications bot (Jan 20 2022 at 03:09):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 03:09):cfallin created PR review comment:
Good idea! This actually shaved the 62MB below down to 61MB.
Wasmtime GitHub notifications bot (Jan 20 2022 at 03:10):cfallin updated PR #3699 from epoch-interruption to main.
Wasmtime GitHub notifications bot (Jan 20 2022 at 03:13):cfallin updated PR #3699 from epoch-interruption to main.
Wasmtime GitHub notifications bot (Jan 20 2022 at 15:20):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 15:20):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 15:20):alexcrichton created PR review comment:
instrumentatino
Wasmtime GitHub notifications bot (Jan 20 2022 at 15:20):alexcrichton created PR review comment:
I might rephrase this from "it would be better to have..." to something focusing on how epochs are an alternative to fuel which is more lightweight. This seems to imply that fuel isn't really that useful but it's still useful for the determinism aspect (which doesn't necessarily need to be mentioned here)
Wasmtime GitHub notifications bot (Jan 20 2022 at 15:20):alexcrichton created PR review comment:
The docs here look great to me, thanks!
But yeah if you'd be up for adding an epoch-equivalent example that'd be awesome.
Wasmtime GitHub notifications bot (Jan 20 2022 at 21:39):cfallin updated PR #3699 from epoch-interruption to main.
Wasmtime GitHub notifications bot (Jan 20 2022 at 21:44):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 20 2022 at 21:44):cfallin created PR review comment:
I just measured this (for future:
--epoch-interruptionon bothwasmtime compileandwasmtime run, and hacksrc/commands/run.rsto set a nonzero epoch deadline on the store, then build .cwasm and run it). It seems to be in the noise at least on SpiderMonkey running a CPU-intensive workload (fib()) -- this tells me that the regalloc is doing the right thing and spilling where it needs to, so it's reasonable to use aVariablehere as you suggest for slightly simpler/smaller IR, I think. Thanks!
Wasmtime GitHub notifications bot (Jan 20 2022 at 21:54):cfallin updated PR #3699 from epoch-interruption to main.
Wasmtime GitHub notifications bot (Jan 20 2022 at 21:58):cfallin updated PR #3699 from epoch-interruption to main.
Wasmtime GitHub notifications bot (Jan 20 2022 at 22:45):cfallin merged PR #3699.
Last updated: Nov 22 2024 at 17:03 UTC