[wasm] Implement MINT_SWITCH opcode in jiterpreter #107423
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce runtime option for max switch size Disable trace generation once the trace table fills up, since there's no point to it
Contributor
|
Tagging subscribers to this area: @BrzVlad, @kotlarmilos |
kg
requested review from
lambdageek,
lewing,
pavelsavara and
steveisok
as code owners
pavelsavara
approved these changes
Sep 6, 2024
Member
pavelsavara
left a comment
pavelsavara
left a comment
There was a problem hiding this comment.
@BrzVlad could you please review the logic please ?
| mono_assert(getU16(ip) === MintOpcode.MINT_SWITCH, "decodeSwitch called on a non-switch"); | ||
| const n = getArgU32(ip, 2); | ||
| const result = []; | ||
| /* |
BrzVlad
approved these changes
Sep 6, 2024
This was referenced Sep 11, 2024
Closed
jtschuster
pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
* Implement MINT_SWITCH opcode (without support for backward jumps) * Introduce runtime option for max switch size (set to 0 to disable switches) * Disable trace generation once the trace table fills up, since there's no point to it
sirntar
pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
* Implement MINT_SWITCH opcode (without support for backward jumps) * Introduce runtime option for max switch size (set to 0 to disable switches) * Disable trace generation once the trace table fills up, since there's no point to it
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements basic support for switches in the jiterpreter, with a configurable size limit (big ones would eat up all the available trace space) that can be set to 0 in order to disable them.
Switch targets that go backwards (Vlad says these should be exceedingly rare) will cause a runtime bailout, as will switches that are too big. In local testing these bailouts are very rare, even with the current low threshold.
If a switch targets places outside of the trace, only those switch targets that are outside will cause bailouts - the rest of them will still work. So even for large traces where half the switch arms aren't reachable, this is still an improvement over how it was before, I think.
In the future if we raise the max trace size we can also raise the size limit on switches - from looking at instrumentation, a limit of 64 or so is probably ideal.
This PR also fixes a bug I noticed - if the jiterpreter trace table filled up we were still generating+compiling a trace and then failing to put it into the table, which wasted CPU time.