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] Make jiterpreter module size limit configurable + clean up technical debt #107318

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

kg
Copy link
Member

@kg kg commented Sep 3, 2024

At present the jiterpreter has a hard-coded module size limit of approximately 4KB. This is because Chrome used to limit synchronously compiled modules to this size. As of Chrome 114 this is no longer the case (they raised the limit to a much, much bigger value we'll never hit.)

This PR makes the module size limit a configurable runtime option and cleans up some technical debt in the process (a couple missing bounds checks in the code generator, etc.)

…ng jiterp codegen

Add missing overflow checks to blob builder appends
Make maximum trace size a runtime option
Move blob builder capacity to a named constant near the top of the file
Copy link
Contributor

Tagging subscribers to this area: @BrzVlad, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

@kg
Copy link
Member Author

kg commented Sep 3, 2024

Incidentally, I tested size limits of 40960 and 16384 against the current limit of 4096. A size limit of 16384 seems to be a good balance, where it reduces the number of traces compiled while significantly reducing the number of bailouts caused by failed branches. The amount of code generated goes up, but it doesn't seem to meaningfully increase time spent compiling or generating WASM:

40960:
// jitted 2713109 bytes; 2736 traces (88.5%) (878 rejected); 0 jit_calls; 0 interp_entries
// cknulls eliminated: 17291, fused: 1635; back-branches emitted: 2407, failed: 336 (87.8%);
// time: 312ms generating, 297ms compiling wasm.
// traces bailed out 8229120 time(s) due to Branch
// traces bailed out 12426624 time(s) due to BackwardBranch
// traces bailed out 175474 time(s) due to ComplexBranch

16384:
// jitted 2702856 bytes; 2736 traces (88.5%) (879 rejected); 0 jit_calls; 0 interp_entries
// cknulls eliminated: 17125, fused: 1642; back-branches emitted: 2386, failed: 340 (87.5%);
// time: 319ms generating, 289ms compiling wasm.
// traces bailed out 8227881 time(s) due to Branch
// traces bailed out 12308567 time(s) due to BackwardBranch
// traces bailed out 217803 time(s) due to ComplexBranch

4096:
// jitted 2469105 bytes; 2774 traces (88.6%) (879 rejected); 0 jit_calls; 0 interp_entries
// cknulls eliminated: 13679, fused: 1618; back-branches emitted: 2137, failed: 355 (85.8%);
// time: 316ms generating, 316ms compiling wasm.
// traces bailed out 19168565 time(s) due to Branch
// traces bailed out 14637218 time(s) due to BackwardBranch
// traces bailed out 216957 time(s) due to ComplexBranch

I think it would make sense to raise the limit in NET10 to at least 16KB, maybe more, as a step towards reducing the limit entirely. The logic for the limit is really complicated and could likely be replaced with something much simpler to remove some technical debt from the jiterpreter.

@kg kg merged commit 57a4b04 into dotnet:main Sep 4, 2024
78 of 81 checks passed
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Sep 6, 2024
…hnical debt (dotnet#107318)

Fix spurious "block stack not empty" errors when an error occurs during jiterp codegen
Add missing overflow checks to blob builder appends
Make maximum trace size a runtime option
Move blob builder capacity to a named constant near the top of the file
jtschuster pushed a commit to jtschuster/runtime that referenced this pull request Sep 17, 2024
…hnical debt (dotnet#107318)

Fix spurious "block stack not empty" errors when an error occurs during jiterp codegen
Add missing overflow checks to blob builder appends
Make maximum trace size a runtime option
Move blob builder capacity to a named constant near the top of the file
sirntar pushed a commit to sirntar/runtime that referenced this pull request Sep 30, 2024
…hnical debt (dotnet#107318)

Fix spurious "block stack not empty" errors when an error occurs during jiterp codegen
Add missing overflow checks to blob builder appends
Make maximum trace size a runtime option
Move blob builder capacity to a named constant near the top of the file
@github-actions github-actions bot locked and limited conversation to collaborators Nov 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants