Generate ReadyToRun unboxing stubs for browser Wasm - #133218
Merged
Merged
Conversation
Replace per-method JIT-generated unboxing bodies with shared structural Wasm stubs and carry their targets in Wasm-specific portable entrypoint metadata. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5928723d-b2b5-4d4a-a22d-703533474aa0
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @dotnet/crossgen-contrib |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A newly added sortable node (WasmUnboxingStubNode) has an incomplete CompareToImpl ordering that can violate total ordering and lead to non-deterministic outputs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a new browser-Wasm ReadyToRun (R2R) strategy for unboxing stubs: instead of emitting per-method stub bodies, it generates a small set of shared, string-discoverable Wasm assembly stubs and wires the runtime to bind each unboxing MethodDesc to its shared stub plus target metadata.
Changes:
- Adds runtime support for resolving and publishing shared Wasm unboxing stubs, including a Wasm-specific portable-entrypoint layout that stores the target MethodDesc and target portable entrypoint just before the stub’s PortableEntryPoint.
- Extends Wasm signature key generation to support a “structural Wasm type” encoding used for unboxing stub lookup keys (
U,UG,UM, with optional return-buffer marker handling). - Adds Crossgen2 node generation + tests validating the presence of the new string-discoverable unboxing stub keys in Wasm-targeted R2R images.
File summaries
| File | Description |
|---|---|
| src/coreclr/vm/wasm/helpers.hpp | Declares GetUnboxingStub for runtime lookup of pregenerated Wasm unboxing stubs. |
| src/coreclr/vm/wasm/helpers.cpp | Implements structural Wasm signature-key computation and GetUnboxingStub. |
| src/coreclr/vm/prestub.cpp | Hooks unboxing-stub prestub flow on Wasm to publish shared stub + target metadata. |
| src/coreclr/vm/precode_portable.hpp | Adds UnboxingStubPortableEntryPoint with two pre-PEP target fields on Wasm. |
| src/coreclr/vm/method.cpp | Allocates/initializes the Wasm unboxing-stub portable-entrypoint wrapper when needed. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj | Includes the new WasmUnboxingStubNode source. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs | Generalizes rooting helper to accept DependencyNodeCore<NodeFactory>. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs | Produces shared Wasm unboxing stub nodes keyed by structural signature/kind/retbuf marker. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmUnboxingStubNode.cs | New node that emits the shared Wasm unboxing stub bodies and lookup keys. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/R2RResultChecker.cs | Adds assertions for string-discoverable thunk keys in R2R images. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCases/VirtualMethodGenerics/NonGVM.cs | Adds a test case to differentiate return-buffer vs explicit-arg structural signatures. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCases/VirtualMethodGenerics/GVM.cs | Tweaks a test method body to drive Wasm GVM-related unboxing stub coverage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCases/R2RTestSuites.cs | Adds Wasm-targeted test suites validating U/UG/UM keys. |
| docs/design/coreclr/botr/readytorun-format.md | Documents unboxing-stub key formats and the Wasm portable-entrypoint layout. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
Include the target Wasm type in CompareToImpl so distinct stub nodes cannot compare equal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5928723d-b2b5-4d4a-a22d-703533474aa0
jkotas
reviewed
Sep 4, 2026
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes CoreCLR prestub/portable-entrypoint behavior and introduces new Wasm ABI-dependent stub generation, which warrants careful human review for cross-thread publication and calling-convention correctness.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
Treat extended unboxing entrypoints and their prestub path as part of FEATURE_PORTABLE_ENTRYPOINTS instead of adding Wasm-specific guards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5928723d-b2b5-4d4a-a22d-703533474aa0
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There is a debug-contract mismatch in UnboxingStubPortableEntryPoint::SetStubTargetAndActualCode (LIMITED_METHOD_CONTRACT calling STANDARD_VM_CONTRACT) that can trigger contract violations and should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
jkotas
reviewed
Sep 4, 2026
Member
|
LGTM otherwise |
This was referenced Sep 4, 2026
BrzVlad
approved these changes
Sep 4, 2026
Use the correct VM wrapper contract, document the generated-stub offset invariants, and clarify unboxing stub lookup terminology. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5928723d-b2b5-4d4a-a22d-703533474aa0
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes CoreCLR prestub/portable-entrypoint behavior and introduces a new runtime/compiler contract (fixed-offset metadata + structural keys) that warrants focused human review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/coreclr/vm/wasm/helpers.cpp:1935
- GetUnboxingStub returns nullptr on several paths without initializing the out parameters. Today the only caller guards on the return value, but leaving outputs undefined makes the API easy to misuse (and can turn into a latent bug if a future caller reads the outputs on failure). Initialize the outputs to null/0 up-front so all failure paths are safe.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/R2RResultChecker.cs:47 - HasStringThunkWithPrefix has a special-case for prefix "U" that deliberately excludes "UG"/"UM" keys, so the method name/contract is a bit misleading and the predicate is hard to read. This makes it easy for future test authors to call it expecting plain StartsWith semantics. Consider rewriting the logic with an explicit branch (or documenting the special-case) so the intent is obvious.
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
Member
|
Conflicts need resolving |
Resolve conflicts by preserving upstream portable-thunk changes and the generated Wasm unboxing stub integration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5928723d-b2b5-4d4a-a22d-703533474aa0
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The unboxing-stub portable entrypoint layout relies on fixed offsets, and one additional static_assert is needed to fully enforce that contract.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
jkotas
reviewed
Sep 5, 2026
jkotas
reviewed
Sep 5, 2026
jkotas
reviewed
Sep 5, 2026
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes low-level VM entrypoint publishing and adds new crossgen/runtime ABI coupling on Wasm where subtle regressions (ordering/layout/call-convention) warrant a maintainer’s deep review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
jkotas
approved these changes
Sep 8, 2026
Member
|
There are still two pieces of unaddressed minor feedback above |
Member
|
this broke things #133491 |
davidwrighton
added a commit
that referenced
this pull request
Sep 9, 2026
## Summary - use `STANDARD_VM_CONTRACT` for `GetUnboxingStub` - keep the generated unboxing stub typed as `void*` by reusing `LookupPortableEntryPointThunk` - address the remaining minor review feedback from #133218 ## Validation - `build.cmd clr -c release -os browser` - focused `WasmVirtualMethodGenerics` ReadyToRun tests: 2 passed > [!NOTE] > This pull request description was generated with GitHub Copilot. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
lewing
added a commit
that referenced
this pull request
Sep 10, 2026
…133516) <!-- --> Fixes browser-Wasm CoreCLR ReadyToRun failures introduced after #133218 while preserving shared `U`, `UG`, and `UM` unboxing stubs and almost all of their binary-size benefit. ## Corrected root-cause analysis The initial version of this PR replaced `U`/`UG` with per-method managed thunks based on an ABI-mismatch hypothesis. That approach and its portable-prestub changes have been removed. The revised patch does not change the shared-stub calling conventions or lookup keys. NESM inspection stopped immediately before the failing Test23 `call_indirect`: the call expected five `i32` parameters, but the selected table slot was zero (a null function reference). The expected parameter count alone did not establish a `U` versus `UG` mismatch. The observed delay-load fixup resolved to a non-unboxing shared target requiring a generic context, whose portable entrypoint lacked an R2R-to-interpreter adapter such as `IvTiS1p`. Replacing a compiled per-method unboxing thunk with a structural assembly stub lost dependencies previously introduced by compiling the thunk's managed body. There are two distinct transitions: - **Interpreter -> unboxing stub:** needs an `M` adapter for the full managed unboxing signature, for example `MS56Tp`. - **Unboxing stub -> target:** needs an `I` adapter for the target signature, including its generic context where applicable, for example `IS56Tip` or `IvTiS1p`, so its portable entrypoint is callable before the target's native body is published. Structural sharing also allows the runtime to find a shared stub for a generic instantiation Crossgen2 never compiled. A matching Wasm function shape does not imply that the exact managed transition cookie exists: different struct sizes can share the same structural stub but require different interpreter adapters. The actual `GitHub_19361` execution exposed this case with a missing `MS56Tp` cookie. ## Changes - Add a per-target dependency node retaining the shared stub, compiled target, interpreter-to-R2R adapter for the unboxing signature, and R2R-to-interpreter adapter for the target signature. - Keep these dependencies per managed target rather than on the structurally shared node, so signatures with the same Wasm shape but different managed layouts retain their distinct adapters. - Before publishing a shared unboxing stub, require the exact incoming interpreter cookie and callable code in the target portable entrypoint. An installed R2R-to-interpreter adapter counts as callable code; this does not require a precompiled native target body. - If either transition is unavailable, use the existing interpreted IL-stub fallback. R2R callers independently root their call-signature `I` adapters through the existing call-site recording machinery. - Extend the existing Wasm generic-dispatch tests with a 56-byte struct return alongside the 16-byte case, checking distinct adapters while retaining the original shared-stub assertions. ## Evidence and validation Browser-Wasm **Release CoreCLR under Node.js 26.4.0**, with explicitly regenerated and Wasm-validated CoreLib, LINQ, and test images: | Runtime case | Actual execution evidence | Result | |---|---|---| | `LoaderClassloaderGenerics`, `DisplayName~genrecur.dll` | `Test23 OK`, matching `Passed test` line | Exit 100 | | `Regressions`, `DisplayName~genrecur.dll` | `Test23 OK`, matching `Passed test` line | Exit 100 | | `Regression_NoOptimize_r_1`, `Repro.Program.TestEntryPoint` | `Starting stress loop`, `Result: Completed Normally`, matching `Passed test` line | Exit 100 | The earlier `GitHub_19361` name filter matched no test; earlier success claims using that filter were invalid. The results above use the actual fully qualified method name and confirm that the stress loop executed. Earlier stale CoreLib images were also replaced explicitly rather than relying on layout generation to refresh them. Additional validation: - Browser Release runtime and corerun build: succeeded. - Wasm `ILCompiler.ReadyToRun.Tests`: 75 passed, 37 target-inapplicable skips. - Host macOS arm64 generic-dispatch tests: 3 passed, 2 Wasm-only skips. - Negative dependency checks: removing only `I(target)` fails on missing `IS16Tip`; removing only `M(unboxing)` fails on missing `MS56Tp`. Restoring both passes. The full CI outer-loop matrix and a browser-hosted run have not been rerun locally. ## Binary size Optimized browser `System.Private.CoreLib` images using `--optimize --generate-unboxing-stubs` in the local comparison: | Variant | Bytes | |---|---:| | Earlier fully shared prototype | 26,991,640 | | Revised shared stubs with transition dependencies | 26,993,234 | | Superseded per-method `U`/`UG` fallback | 27,387,567 | The revised image adds **1,594 bytes** over the earlier fully shared prototype and saves **394,333 bytes** compared with the superseded fallback, retaining approximately **99.6%** of the size reduction in that comparison. This supersedes the initial description's claim that the fix gives back nearly the entire saving. Fixes #133491 > [!NOTE] > This pull request description and investigation summary were generated with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
U,UG, andUMstub forms for ordinary, MethodTable-context, and MethodDesc-context unboxingSize impact
Measured by compiling the same Release
System.Private.CoreLib.dllwith Release browser-Wasm Crossgen2 before and after this change using--optimize --generate-unboxing-stubs.Validation
build.cmd clr -c release -os browserWasmVirtualMethodGenericsReadyToRun tests: 2 passedNote
This pull request description was generated with GitHub Copilot.