Add Store::call_hook API
#1083
Comments
Robbepop
added
enhancement
|
I have a use case for call_hook, so I figured I could take a shot at this. I now have a working version of call_hook, but had one question. Should call_hook be behind a feature flag? It seems like a change is coming to wasmtime to put it behind a feature flag, which should make it easier to make sure that this has no/minimal performance impact where it is not needed. I have tried benchmarking with my changes locally and could see some regressions, but I also saw 20% speedups that I can't explain, so I don't know how much I can trust the benchmark numbers on my computer. |
|
Hi @emiltayl , thank you for working on this feature. Can you please share a link to the discussion for putting this behind a feature flag? I would like to know more about the reasoning. |
|
The pull request for creating the feature flag is at bytecodealliance/wasmtime#8795, and at bytecodealliance/wasmtime#8808 they disable it by default. It seems to be merged to main, but not put in a release yet if I understand correctly. I agree that wasmi's performance shouldn't be impacted to a large degree by this change in theory. I don't have the code in front of me right now, but I will be back with the regressions from the benchmarks tomorrow. |
|
I've included the benchmarks that reported regressions sorted by % change. Judging by the changes I've made, I would have expected to see most change in In the instantiate benchmarks an extra Edit I tried running the benchmarks again on main, comparing with the first benchmark and got comparable results to my call_hook branch, with some benchmarks unexpectedly performing much poorer or slightly better. Seems to be a problem with me running the benchmarks locally. The overhead, especially when no call hook is set, should be quite low. |
|
Hi @emiltayl and sorry for my late reply. The benchmarks indeed do not look all too good all in all but most of the regressions fortunately are not in the Wasmi executor but in the Wasm module instantiation. I am not sure how we shall continue with this ideally. The best is if you could open a PR so we could discuss the technical details there but at this point I won't guarantee to merge the efforts if we cannot resolve the regressions or at least soften them. |
* Start implementing `Store::call_hook` (#1083) * Add the `Store::call_hook` function, which stores a call hook in `Store` * Create tests to verify call hook behavior * Let call hooks return any `wasmi::Error` * Changed signature of call hooks to return a `wasmi::Error` instead of a `TrapCode`. * Use `assert_eq!` instead of `assert!` for tests, cosmetic change * Invoke call hooks on function calls * An `invoke_call_hook` function in Store to avoid problems with the borrow checker - we need a reference to the underlying data and the stored call hook * Invoke call hook on host -> wasm and wasm -> host calls * Clarified documentation on error propagation for call hooks * Fixed attribute placement and clippy errors * Placed attributes after doc comments * Added hint to allow complex type for `generate_error_after_n_calls` * Fixed clippy warnings on implicit conversion of ! to () in call_hook tests * Help the compiler optimize function calls for the no call hook scenario * Inline `Store::invoke_call_hook` to avoid extra function call * Add a `Store::invoke_call_hook_impl` function that is `#[cold]` to hint that the compiler should optimize for the scenario that there are no call hooks * Update crates/wasmi/src/store.rs Co-authored-by: Robin Freyler <[email protected]> --------- Co-authored-by: Robin Freyler <[email protected]>
|
Closed with #1144 being merged. |
This is about adding a
Store::call_hookAPI similar to Wasmtime'sStore::call_hookin order to improve Wasmi's Wasmtime API mirroring.}This should not regress performance of host function calls or the Wasmi executor generally.
The text was updated successfully, but these errors were encountered: