winch: Implement Fuel-Based Interruption #9472
Conversation
Subscribe to Label Action
This issue or pull request has been labeled: "winch"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
There was a problem hiding this comment.
Looks reasonable to me, thanks for this!
One thought I had reading over this was that this should probably update some documentation somewhere around enabling fuel that Winch is supported. I don't think that's quite right though since we don't exhaustively document Winch's support on all other Config options, so documenting this as a one-off doesn't feel right. That being said it led me to another thought though that there was no change here associated with "ok actually allow Winch to use fuel". This means that it looks like we were silently accepting fuel-enabled-Config even though Winch didn't support it.
To handle that, either here or as a follow-up, mind updating the Compiler for ... in winch-codegen? IIRC there's a method set_tunables and it might be good to have Winch-specific logic to reject configurations that Winch doesn't support. For example Winch doesn't support generate_native_debuginfo or epoch_interruption. This might help move some code out of Config::validate as well and keep it in a Winch-specific location.
|
Thanks for the review @alexcrichton.
I think it's reasonable to audit the |
This commit introduces proper validation and documentation to handle the engine features not currently supported by Winch (e.g., `consume_fuel`, follow-up to the discussion in bytecodealliance#9472). In this commit, the tunables validation is done at the `set_tunables` method in Winch's `wasmtime_environ::compile::CompilerBuilder` implemetation, which enables removing Winch specific logic from `Config::validate` This change also introduces the question of how to consolidate the compiler specific flags with Winch (e.g, the user-specified `cranelift_*` options) given that not all of them are applicable to Winch (e.g., `cranelift_debug_verifier`, `cranelift_nan_canonicalization`), this change doesn't introduce any functionality on this front, however, it's probably something worth considering/discussing.
|
@alexcrichton I've opened #9490 -- once #9490 lands, I'll rebase and land this change. |
This commit introduces proper validation and documentation to handle the engine features not currently supported by Winch (e.g., `consume_fuel`, follow-up to the discussion in bytecodealliance#9472). In this commit, the tunables validation is done at the `set_tunables` method in Winch's `wasmtime_environ::compile::CompilerBuilder` implemetation, which enables removing Winch specific logic from `Config::validate` This change also introduces the question of how to consolidate the compiler specific flags with Winch (e.g, the user-specified `cranelift_*` options) given that not all of them are applicable to Winch (e.g., `cranelift_debug_verifier`, `cranelift_nan_canonicalization`), this change doesn't introduce any functionality on this front, however, it's probably something worth considering/discussing.
* winch: Improve tunables/configuration validation This commit introduces proper validation and documentation to handle the engine features not currently supported by Winch (e.g., `consume_fuel`, follow-up to the discussion in #9472). In this commit, the tunables validation is done at the `set_tunables` method in Winch's `wasmtime_environ::compile::CompilerBuilder` implemetation, which enables removing Winch specific logic from `Config::validate` This change also introduces the question of how to consolidate the compiler specific flags with Winch (e.g, the user-specified `cranelift_*` options) given that not all of them are applicable to Winch (e.g., `cranelift_debug_verifier`, `cranelift_nan_canonicalization`), this change doesn't introduce any functionality on this front, however, it's probably something worth considering/discussing. * Update the fuzzers * Use `wasmtime_test` instead of test This allows a couple of things: * Pre-configuring `Winch` as the strategy * Handling the target architetures supported by Winch
Closes: bytecodealliance#8090 This commit introduces the initial implementation of fuel-based interruption in Winch. To maintain consistency with existing fuel semantics, this implementation closely follows the Wasmtime/Cranelift approach, with the following exception: * Local Fuel Cache: Given Winch's emphasis on compilation speed, this implementation does not optimize for minimizing loads and stores. As a result, checking and incrementing fuel currently requires explicit loads and stores. Future optimizations may be considered to improve this aspect. This commit also includes a small refactoring in the visitor, which introduces more generic "visitor hook" which enable handling the invariants that need to happen before and after emitting machine code for each Wasm operator.
saulecabrera
force-pushed
the
winch-fuel
branch
from
678d999 to
878f238
Compare
This was implemented in bytecodealliance#9472, so let's let the fuzzers have a go at it.
This was implemented in #9472, so let's let the fuzzers have a go at it.
This commit is a follow-up to bytecodealliance#9472. It ensures that the fuel checking code makes use of `Context::without` to retrieve the current fuel register. Even though no issues have been found, it is preferable to use `Context::without` in scenarios in which we have to introduce special out-of-band control flow and function calls to ensure that all the register allocation invariants are respected i.e., avoid holding onto to registers that: are not in the stack and that might be needed for function calls.
This commit is a follow-up to #9472. It ensures that the fuel checking code makes use of `Context::without` to retrieve the current fuel register. Even though no issues have been found, it is preferable to use `Context::without` in scenarios in which we have to introduce special out-of-band control flow and function calls to ensure that all the register allocation invariants are respected i.e., avoid holding onto to registers that: are not in the stack and that might be needed for function calls.
Closes: #8090
This commit introduces the initial implementation of fuel-based interruption in Winch.
To maintain consistency with existing fuel semantics, this implementation closely follows the Wasmtime/Cranelift approach, with the following exception:
This commit also includes a small refactoring in the visitor, which introduces more generic "visitor hook" which enable handling the invariants that need to happen before and after emitting machine code for each Wasm operator.