Stream: git-wasmtime

Topic: wasmtime / PR #3699 Add epoch-based interruption for coop...


view this post on Zulip 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 Engine performs an
engine.increment_epoch() call periodically, e.g. once per millisecond.
An async invocation of a Wasm guest on a Store can 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 the Store, just as for fuel.) This call does only
signal-safe work (it increments an AtomicU64) so could be invoked from
a periodic signal, or from a thread that wakes up once per period.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 19 2022 at 01:36):

cfallin requested alexcrichton for a review on PR #3699.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):

alexcrichton submitted PR review.

view this post on Zulip 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 Trap is 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.

view this post on Zulip 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.

view this post on Zulip 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 unsafe here 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.

view this post on Zulip 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?

view this post on Zulip 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 Config method 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 think Config methods 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)

view this post on Zulip 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:

view this post on Zulip Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):

alexcrichton created PR review comment:

Could this link to the Config documentation for epochs?

view this post on Zulip 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 Config documentation for epochs?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):

alexcrichton created PR review comment:

The naming here and with refill_epoch_deadline is a little confusing to me, perhaps something like:

and along the lines of a different comment if set_epoch is called (which should be available without the async feature I think) and set_epoch_yield_delta isn't called then we can trap on the first epoch change)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):

alexcrichton created PR review comment:

Could this use $foo for names of functions?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):

alexcrichton created PR review comment:

Can this be passed as an Option instead 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)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 19 2022 at 15:48):

alexcrichton created PR review comment:

One idea perhaps is for new_epoch to return the new epoch so a reload doesn't have to happen afterwards to cut down on the code size slightly.

view this post on Zulip 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::Variable to avoid having to reload it?

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:07):

cfallin updated PR #3699 from epoch-interruption to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:07):

cfallin submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:07):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:07):

cfallin created PR review comment:

Nice, had no idea about this -- added!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:08):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:08):

cfallin created PR review comment:

Added!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:08):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:08):

cfallin created PR review comment:

Added a comment describing the safety here.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:09):

cfallin updated PR #3699 from epoch-interruption to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:11):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:11):

cfallin created PR review comment:

I've renamed the methods to set_epoch_deadline for the "set deadline to N ticks in the future" action (analogous to add_fuel), and added epoch_deadline_trap and epoch_deadline_async_yield_and_update to configure the two different behaviors when deadline is reached.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:11):

cfallin created PR review comment:

Updated docs here and elsewhere!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:11):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:12):

cfallin submitted PR review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:52):

cfallin updated PR #3699 from epoch-interruption to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:52):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:52):

cfallin created PR review comment:

Fixed -- this is now async_yield_impl and the fuel update happens in out_of_gas (one of its callers) instead.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:53):

cfallin created PR review comment:

Added all of these -- thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:53):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:53):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:53):

cfallin created PR review comment:

Done!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:54):

cfallin submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:59):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 02:59):

cfallin created PR review comment:

Just tested with a spidermonkey.wasm I had laying around: baseline (no fuel or epoch-interruption) produced a 54MB .cwasm via wasmtime compile; with --consume-fuel I get 68MB; with --epoch-interruption I get 62MB.

So on that wasm module, fuel instrumentation has a 26% overhead while epoch instrumentation has a 15% overhead.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 03:09):

cfallin submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 03:10):

cfallin updated PR #3699 from epoch-interruption to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 03:13):

cfallin updated PR #3699 from epoch-interruption to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 15:20):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 15:20):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 15:20):

alexcrichton created PR review comment:

instrumentatino

view this post on Zulip 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)

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 21:39):

cfallin updated PR #3699 from epoch-interruption to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 21:44):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 21:44):

cfallin created PR review comment:

I just measured this (for future: --epoch-interruption on both wasmtime compile and wasmtime run, and hack src/commands/run.rs to 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 a Variable here as you suggest for slightly simpler/smaller IR, I think. Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 21:54):

cfallin updated PR #3699 from epoch-interruption to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 21:58):

cfallin updated PR #3699 from epoch-interruption to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 20 2022 at 22:45):

cfallin merged PR #3699.


Last updated: Dec 23 2024 at 12:05 UTC