Wasmtime GitHub notifications bot (Jun 13 2024 at 19:38): Wasmtime GitHub notifications bot (Jun 13 2024 at 19:38):alexcrichton opened PR #8795 from alexcrichton:gate-call-hook to bytecodealliance:main:
This commit moves the
Store::call_hookAPI behind a Cargo feature namedcall-hook. This helps speed up the path from wasm into the host by avoiding branches at the start and the end of the execution. In a thread on [Zulip] this is locally leading to significant performance gains in this particular microbenchmark so having an option to disable it at the crate layer seems like a reasonable way to thread this needle for now. This definitely has a downside in that it requires a crate feature at all, but I'm not sure of a better solution as LLVM can't dynamically detect thatStore::call_hookis never invoked and therefore the branch can be optimized away.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Jun 13 2024 at 19:38):alexcrichton requested fitzgen for a review on PR #8795.
Wasmtime GitHub notifications bot (Jun 13 2024 at 19:38):alexcrichton requested wasmtime-core-reviewers for a review on PR #8795.
Wasmtime GitHub notifications bot (Jun 13 2024 at 19:41):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jun 13 2024 at 19:44):alexcrichton updated PR #8795.
Wasmtime GitHub notifications bot (Jun 13 2024 at 19:50):alexcrichton has enabled auto merge for PR #8795.
Wasmtime GitHub notifications bot (Jun 13 2024 at 20:06):alexcrichton merged PR #8795.
Wasmtime GitHub notifications bot (Jun 13 2024 at 21:39):pchickey commented on PR #8795:
I think this is the correct choice - needing call hooks at all is a pretty particular need, and I'd even be fine if this feature was not enabled by default. Have we surveyed if anyone besides Fastly's embedding even makes use of it?
Wasmtime GitHub notifications bot (Jun 13 2024 at 21:39):pchickey edited a comment on PR #8795:
I think this is the correct choice - call hooks fill a pretty particular need, and I'd even be fine if this feature was not enabled by default. Have we surveyed if anyone besides Fastly's embedding even makes use of it?
Wasmtime GitHub notifications bot (Jun 13 2024 at 21:42):alexcrichton commented on PR #8795:
I haven't surveyed myself but carrying a Cargo feature for it isn't too bad. Most of our Cargo features have been relatively low overhead to maintain so far. The main question to me is that this might be an appropriate feature to be off-by-default instead of on-by-default as it can affect benchmarks, but even then it's pretty rare anyone benchmarks host-to-wasm calls or vice versa, most benchmarking is just of the wasm itself.
Wasmtime GitHub notifications bot (Jun 14 2024 at 09:42):tschneidereit commented on PR #8795:
I think regardless of who might encounter this in benchmarks, as long it's not essentially always used, it makes sense to disable by default: we should keep the call overhead low wherever we can, after all, and requiring people to disable very specific features for best performance doesn't seem great to me.
Wasmtime GitHub notifications bot (Jun 14 2024 at 15:06):alexcrichton commented on PR #8795:
I can try to investigate this a bit. I believe the
gcfeature has a much larger impact than call-hooks in terms of performance. I haven't bottomed that out but disablinggc-by-default I think would be much more significant than disablingcall-hookby default.
Wasmtime GitHub notifications bot (Jun 14 2024 at 15:25):alexcrichton commented on PR #8795:
Measuring locally from the benchmark from Zulip, which is very heavy in wasm->host calls and the host call itself does nothing, enabling the
gcfeature of Wasmtime yields a slowdown of 3.5x and enabling thecall-hookfeature slows down only 30%.Put another way I don't think it's worth over-rotating too much on
call-hook,gcis the main feature that's causing a slowdown.
Wasmtime GitHub notifications bot (Jun 14 2024 at 18:07):alexcrichton commented on PR #8795:
With https://github.com/bytecodealliance/wasmtime/pull/8807 the performance is on-par now with the
gcfeature enabled, so I'll follow that up by disabling thecall-hookfeature by default.
Last updated: Nov 22 2024 at 16:03 UTC