elliottt requested fitzgen for a review on PR #8540.
elliottt requested wasmtime-core-reviewers for a review on PR #8540.
elliottt opened PR #8540 from elliottt:trevor/tail-calls-default
to bytecodealliance:main
:
Switch to enabling the wasm tail call feature by default when the target is one of
x86_64
,aarch64
, orriscv64
, and the compilation strategy iscranelift
. We also added tests checking that the feature is not enabled for either winch or thes390x
backend.Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
<!--
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
-->
elliottt updated PR #8540.
elliottt requested alexcrichton for a review on PR #8540.
elliottt updated PR #8540.
alexcrichton submitted PR review:
Nice! Out of curiosity do you know if a sightglass run has been done to confirm there's no perf regression?
Otherwise though I see how this is a little tricky to handle this. I'm wary of how this is currently implemented. We've tried to avoid interactions where one feature silently enables another or interacts with another and this is adding a bit of that with relation to the tail call handling.
Could the validation/etc be done without needing
mut self
onConfig::validate
? Instead could this preserve the&self
-ness and perhaps add other validations to ensure the stars align right?
elliottt commented on PR #8540:
I did some perf testing during the initial tail-call refactoring PRs and didn't notice any regression in perf. I'll do another sightglass run with this branch to verify that.
We initially tried to implement this in
Config::new
, but as we still have some cases that don't permit tail calls, it ended up being easiest to interpret the default invalidate
. I agree that the conditions are a little unfortunate, but I think this is the best option currently.
alexcrichton commented on PR #8540:
Do you think it would be workable to enable tail calls by default except for s390x and then have a validation check that tail calls aren't enabled for winch? That would require
-W tail-call=n
for running with Winch but that I think would compose a bit better
elliottt commented on PR #8540:
Where would we set the default for
tail-call
that isn'tConfig::new
? The problem with setting it there is that we don't yet know the compilation target. That was part of what motivated delaying the interpretation of the default toConfig::validate
, as we would definitely know what target we were compiling for at that point. Otherwise, I do like the approach of explicitly disabling tail calls when also targeting winch.
fitzgen commented on PR #8540:
Nice! Out of curiosity do you know if a sightglass run has been done to confirm there's no perf regression?
Yep, there were multiple runs at various points here, and there is no longer any gap between
tail
andwasmtime-systemv
anymore. In fact, I'm 99% sure the two calling conventions are identical when there aren't any actual tail calls.Could the validation/etc be done without needing
mut self
onConfig::validate
? Instead could this preserve the&self
-ness and perhaps add other validations to ensure the stars align right?We tried to do this in
Config::new
initially, but because we don't know the compilation strategy (cranelift vs winch) or target (s309x or not) yet, we therefore don't know what default value to choose.The only alternatives I see are
Checking these conditions when setting every related knob (i.e. check that the target isn't set to s390x when enabling tail calls, check that tail calls aren't set when configuring s390x as a target, etc...). This is annoying because there are now many checks to keep in sync with each other.
Enable tail calls by default, don't change any other logic. This effectively will require that s390x and winch configs explicitly disable tail calls, which is kind of annoying.
Move the choose-the-default-if-it-wasn't-explicitly-configured logic we added here out of
validate
and into afinalize_defaults
method or something that is called just beforevalidate
. This wayvalidate
would still be&self
and we would have a clear division between validation and default computation. But this is sort of just rearranging deck chairs.Don't enable tail calls by default until all configs can support them. This is pretty suboptimal. Do we just wait for s390x? Or winch too? This could be a long time and I really want to start leveraging all the good work that has gone into this feature, deleting the old calling conventions, and all that. FWIW, we have prior art of enabling wasm features by default on some targets but not others when they aren't ready there yet (reference types, for example).
alexcrichton commented on PR #8540:
Ah sorry yeah I was thinking
Config::new
would be the place, but I always forget about cross-compilation which is why acfg!
for s390x wouldn't work. I also agree that all the other alternatives you outlined Nick aren't great, so I'm sold.As a bikeshed, though, how about restructuring this slightly? Something like:
- Leave
validate
as-is with no changes- Add a
fn conditionally_enable_features(&mut self)
method which is called just beforevalidate
- Move this logic to
conditionally_enable_features
Also, in the case that
self.tunables.tail_callable
isNone
I think thatconditionally_enable_features
would be able to do nothing in this case, right? It's only in theNone
case where we infer a proper default from it given other configuration settings?
alexcrichton commented on PR #8540:
Oh also no need to worry about perf, just wanted to double check, sounds like plenty of due diligence
elliottt updated PR #8540.
elliottt requested alexcrichton for a review on PR #8540.
elliottt updated PR #8540.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Mind also editing this to cover that this logic is only applicable if tail calls were not otherwise explicitly disabled or enabled? (aka explaining the
.is_none()
)
elliottt updated PR #8540.
alexcrichton has enabled auto merge for PR #8540.
alexcrichton merged PR #8540.
Last updated: Dec 23 2024 at 13:07 UTC