Stream: git-wasmtime

Topic: wasmtime / PR #8540 Enable the tail calling convention by...

Wasmtime GitHub notifications bot (May 03 2024 at 17:27):

elliottt requested fitzgen for a review on PR #8540.

Wasmtime GitHub notifications bot (May 03 2024 at 17:27):

elliottt requested wasmtime-core-reviewers for a review on PR #8540.

Wasmtime GitHub notifications bot (May 03 2024 at 17:27):

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, or riscv64, and the compilation strategy is cranelift. We also added tests checking that the feature is not enabled for either winch or the s390x 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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (May 03 2024 at 17:27):

elliottt updated PR #8540.

Wasmtime GitHub notifications bot (May 03 2024 at 17:28):

elliottt requested alexcrichton for a review on PR #8540.

Wasmtime GitHub notifications bot (May 03 2024 at 17:29):

elliottt updated PR #8540.

Wasmtime GitHub notifications bot (May 03 2024 at 17:43):

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 on Config::validate? Instead could this preserve the &self-ness and perhaps add other validations to ensure the stars align right?

Wasmtime GitHub notifications bot (May 03 2024 at 17:53):

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 in validate. I agree that the conditions are a little unfortunate, but I think this is the best option currently.

Wasmtime GitHub notifications bot (May 03 2024 at 17:57):

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

Wasmtime GitHub notifications bot (May 03 2024 at 18:05):

elliottt commented on PR #8540:

Where would we set the default for tail-call that isn't Config::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 to Config::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.

Wasmtime GitHub notifications bot (May 03 2024 at 18:07):

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 and wasmtime-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 on Config::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

1. 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.

2. 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.

3. Move the choose-the-default-if-it-wasn't-explicitly-configured logic we added here out of validate and into a finalize_defaults method or something that is called just before validate. This way validate 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.

4. 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).

Wasmtime GitHub notifications bot (May 03 2024 at 18:17):

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 a cfg! 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 before validate
• Move this logic to conditionally_enable_features

Also, in the case that self.tunables.tail_callable is None I think that conditionally_enable_features would be able to do nothing in this case, right? It's only in the None case where we infer a proper default from it given other configuration settings?

Wasmtime GitHub notifications bot (May 03 2024 at 18:18):

alexcrichton commented on PR #8540:

Oh also no need to worry about perf, just wanted to double check, sounds like plenty of due diligence

Wasmtime GitHub notifications bot (May 03 2024 at 18:27):

elliottt updated PR #8540.

Wasmtime GitHub notifications bot (May 03 2024 at 18:28):

elliottt requested alexcrichton for a review on PR #8540.

Wasmtime GitHub notifications bot (May 03 2024 at 18:30):

elliottt updated PR #8540.

Wasmtime GitHub notifications bot (May 03 2024 at 18:34):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 03 2024 at 18:34):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 03 2024 at 18:34):

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())

Wasmtime GitHub notifications bot (May 03 2024 at 18:39):

elliottt updated PR #8540.

Wasmtime GitHub notifications bot (May 03 2024 at 18:49):

alexcrichton has enabled auto merge for PR #8540.

Wasmtime GitHub notifications bot (May 03 2024 at 19:18):

alexcrichton merged PR #8540.

Last updated: Nov 22 2024 at 17:03 UTC