Stream: git-wasmtime

Topic: wasmtime / PR #9737 winch: Epoch-based interruption

Wasmtime GitHub notifications bot (Dec 04 2024 at 23:55):

saulecabrera opened PR #9737 from saulecabrera:winch-epoch to bytecodealliance:main:

Closes https://github.com/bytecodealliance/wasmtime/issues/8091

This commit introduces support for epoch interruption to Winch. The heuristics around epoch check emission are identical to Cranelift's except for the fact that the current implementation doesn't introduce a local-based cache for the current epoch deadline. This is an intentional decision given Winch's focus on compilation performance. However, if needed in the future, knobs could be introduced to optionally introduce a local cache at the cost of reduced compilation performance.

<!--
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 (Dec 04 2024 at 23:55):

saulecabrera requested fitzgen for a review on PR #9737.

Wasmtime GitHub notifications bot (Dec 04 2024 at 23:55):

saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #9737.

Wasmtime GitHub notifications bot (Dec 04 2024 at 23:55):

saulecabrera requested wasmtime-compiler-reviewers for a review on PR #9737.

Wasmtime GitHub notifications bot (Dec 04 2024 at 23:55):

saulecabrera requested wasmtime-core-reviewers for a review on PR #9737.

Wasmtime GitHub notifications bot (Dec 05 2024 at 02:16):

github-actions[bot] commented on PR #9737:

Subscribe to Label Action

cc @fitzgen, @saulecabrera

<details>
This issue or pull request has been labeled: "fuzzing", "winch"

Thus the following users have been cc'd because of the following labels:

• fitzgen: fuzzing
• saulecabrera: winch

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:17):

fitzgen submitted PR review:

Looks great! Just a couple nitpicks inline.

Also I know that most of the testing will be covered by fuzzing and the existing epoch tests, but it may make sense to throw in a single disas test with epochs enabled as well that contains a some ifs, blocks, and a loop just to show that we insert epoch checks only on the loop headers and body prologue. Could also be used to see that deadlines are actually cached, if we ever get around to doing that here.

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:17):

fitzgen created PR review comment:

Is the _var suffix fairly established in Winch? If so, ignore the following.

If not, then I find it a little surprising. I thought maybe there was a Var type that abstracts over registers and stack slots or something. Or introduces a virtual register concept or something like that. I think a _reg suffix would have avoided me going down that line of thought and inquiry, which would make it slightly easier to follow what's going on, as a code reader.

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:17):

fitzgen created PR review comment:

Maybe move this if self.tunables.epoch_interruption into the emit_epoch_check helper? (And rename it to maybe_emit_epoch_check or something like that? Feel free to bikeshed.)

This code motion would simplify callers, which are all doing the exact same conditional.

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:17):

fitzgen created PR review comment:

I think a one-line comment describing what this label is could also be helpful to code readers.

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:17):

fitzgen created PR review comment:

And then I'd expect these to also be _reg if the last suggestion is taken.

Wasmtime GitHub notifications bot (Dec 06 2024 at 13:44):

saulecabrera updated PR #9737.

Wasmtime GitHub notifications bot (Dec 06 2024 at 14:15):

saulecabrera updated PR #9737.

Wasmtime GitHub notifications bot (Dec 06 2024 at 14:17):

saulecabrera commented on PR #9737:

@fitzgen agreed with all the suggestions,thanks! I've also opened https://github.com/bytecodealliance/wasmtime/pull/9737, which applies the suggestions in this PR to fuel checks.

Wasmtime GitHub notifications bot (Dec 06 2024 at 17:10):

saulecabrera updated PR #9737.

Wasmtime GitHub notifications bot (Dec 06 2024 at 17:37):

saulecabrera merged PR #9737.

Last updated: Dec 23 2024 at 12:05 UTC