Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wasm] Fix bugs related to jiterpreter heuristic #99273

Merged
merged 7 commits into from
Mar 7, 2024

Conversation

kg
Copy link
Member

@kg kg commented Mar 5, 2024

While investigating some benchmark regressions I noticed a couple bugs related to the jiterpreter.

I did 3 runs of browser-bench for before/after and the difference is within noise for me, so I don't know whether this makes anything faster or slower. The existing instruction counter is broken in an unpredictable way though, and this correct implementation of it is at least possible to tune.

@ghost
Copy link

ghost commented Mar 5, 2024

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

While investigating some benchmark regressions I noticed a couple bugs related to the jiterpreter.

I did 3 runs of browser-bench for before/after and the difference is within noise for me, so I don't know whether this makes anything faster or slower. The existing instruction counter is broken in an unpredictable way though, and this correct implementation of it is at least possible to tune.

Author: kg
Assignees: -
Labels:

arch-wasm, area-Codegen-Interpreter-mono

Milestone: -

@ghost ghost assigned kg Mar 5, 2024
@kg
Copy link
Member Author

kg commented Mar 5, 2024

Blocked by

[21:37:54] info: Starting:    System.Text.RegularExpressions.Tests.dll
[21:38:40] info: long superbranch detected with opcode 259 (beq.i4.imm.sp) in method <PcreTestData_Cases>d__1.MoveNext
[21:38:40] fail: [MONO] * Assertion at /__w/1/s/src/mono/mono/mini/interp/transform.c:8558, condition `<disabled>' not met
[22:07:50] fail: Tests timed out. Killing driver service pid 80

Which is caused by a 2900-line generator function, the generated state machine seems to have a chance to end up with complex long branches in it depending on how the opcodes end up arranged, and fixing the jiterpreter heuristic permutes the opcodes just so.

We could hide this problem by splitting the generator function in half, but for now we're going to figure out whether there's a better solution.

@kg kg added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 5, 2024
@kg kg removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 6, 2024
@kg kg added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 6, 2024
@kg kg merged commit 5373c01 into dotnet:main Mar 7, 2024
108 of 111 checks passed
akoeplinger added a commit to akoeplinger/runtime that referenced this pull request Mar 10, 2024
This causes a warning. Introduced in dotnet#99273.

```
  src/mono/mono/mini/interp/jiterpreter.c:1686:24: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
  mono_jiterp_is_enabled () {
                         ^
                          void
  1 warning generated.
```
akoeplinger added a commit that referenced this pull request Mar 10, 2024
This causes a warning. Introduced in #99273.

```
  src/mono/mono/mini/interp/jiterpreter.c:1686:24: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
  mono_jiterp_is_enabled () {
                         ^
                          void
  1 warning generated.
```
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Codegen-Interpreter-mono NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants