Wasmtime GitHub notifications bot (Jun 14 2024 at 19:43): Wasmtime GitHub notifications bot (Jun 14 2024 at 19:43):alexcrichton opened PR #8811 from alexcrichton:no-gc-scope-without-gc-values to bytecodealliance:main:
This commit skips the creation of a LIFO scope for GC
Rooted<T>values for hostcalls using statically typed arguments/results (e.g.Func::wrap) where none of the arguments/results have GC values. This removes the last bit of "GC business" onFunc::wrapcalls that don't do anything related to GC.<!--
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 14 2024 at 19:43):alexcrichton requested wasmtime-core-reviewers for a review on PR #8811.
Wasmtime GitHub notifications bot (Jun 14 2024 at 19:43):alexcrichton requested fitzgen for a review on PR #8811.
Wasmtime GitHub notifications bot (Jun 14 2024 at 19:55):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jun 14 2024 at 19:55):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jun 14 2024 at 19:55):fitzgen created PR review comment:
I think this could actually be bad: if I define a host call with only integer params and results, but then create an
externrefinside it[^0], then thatexternrefwill be rooted in the outer LIFO scope, rather than just being rooted in theCaller's scope, which is probably the store's scope. That means theexternrefis going to leak for the whole extent of the program. So I don't think we can actually do this.Is reading a vec's length and then later a well-predicted compare-and-branch to out-of-line code really that expensive?
[^0]: Maybe I then re-entrantly call some other Wasm export, giving it that
externref. Doesn't really matter why I did it, just that I did it.
Wasmtime GitHub notifications bot (Jun 14 2024 at 21:20):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 14 2024 at 21:20):alexcrichton created PR review comment:
My thinking was that it's still safe in that case, right? It means it'll be rooted long but a manual rooting scope can always be created to avoid that.
Perf-wise this was locally showing a 40% perf improvement but I cannot reproduce it against the latest
main. Turns out that I was testing before #8806 landed, and now that that PR has landed this PR is largely perf-neutral. The benchmark here is pretty micro-benchmark-y but at the same time I think it's a nice feature of Wasmtime that the overhead for calling the host is as low as possible, so cleaning up the assembly as much as we can I think is reasonable.
Wasmtime GitHub notifications bot (Jun 14 2024 at 21:23):alexcrichton edited PR review comment.
Wasmtime GitHub notifications bot (Jun 14 2024 at 21:27):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jun 14 2024 at 21:27):fitzgen created PR review comment:
it's a nice feature of Wasmtime that the overhead for calling the host is as low as possible, so cleaning up the assembly as much as we can I think is reasonable.
Agreed in general.
My thinking was that it's still safe in that case, right? It means it'll be rooted longer but a manual rooting scope can always be created to avoid that.
It is memory safe, yes, but it is a fairly big footgun, IMO. At minimum, we would need to be very clear about when a
Calleris vs. isn't a rooting scope in our existing docs around rooting and scopes (I think onRooted? maybe onRootScope? I forget)But if this isn't a perf win anymore, then is it really worth adding the footgun?
Wasmtime GitHub notifications bot (Jun 16 2024 at 21:59):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 16 2024 at 21:59):alexcrichton created PR review comment:
Ok I think that's reasonable yeah. I'll close this and we can revisit if it ever becomes necessary but that seems unlikely too.
Wasmtime GitHub notifications bot (Jun 16 2024 at 21:59):alexcrichton closed without merge PR #8811.
Last updated: Nov 22 2024 at 16:03 UTC