Repository navigation
Use external method thunks for Wasm R2R virtual dispatch - #133146
Conversation
Add signature-specific virtual dispatch thunks and runtime fixup support so eligible ReadyToRun virtual calls can dispatch through vtable offsets instead of the ldvirtftn path. Known design concerns in this prototype: - Ensuring each resolved target is callable from R2R code may not be reliable. - The layering does not replace VSD vtable-stub creation through the existing thunk tables; crossgen2 instead computes a V<sig> thunk for each invoke. - Thunk selection processes an unnecessarily complex logical signature rather than using the Wasm structural signature. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78da0c8f-d377-4b93-99ea-6e442c4a899e
Route ReadyToRun class virtual calls through the existing external-method delay-load pathway instead of encoding a separate virtual-call fixup mode. Resolve the first receiver through ExternalMethodFixupWorker, make its portable entrypoint callable from R2R, and atomically publish a loader-allocator-owned virtual-dispatch portable entrypoint. Preserve the original import entrypoint so dispatch can fall back through normal fixup when a receiver's selected target has not yet been prepared for R2R calls. Discover signature-specific virtual dispatch thunks dynamically through the registered string-thunk table. Emit the required thunk dependencies without embedding them directly in each portable entrypoint, pack both vtable offsets into one word, and use unsigned 16-bit Wasm loads so the dispatch thunk needs only one shared call_indirect. Do not pass VirtualStubCell as a hidden Wasm call argument; the portable entrypoint already identifies the import cell. Also prevent cached-interface initialization from replacing class-virtual import cells on Wasm. Update the ReadyToRun format documentation for the dynamic entrypoint layout and fallback behavior. Validated with a full Debug browser CoreCLR and Release libraries rebuild, regenerated library R2R images, the focused WasmVirtualDispatch R2R test, and the mixed virtualstubdispatch test executing under Node.js with the rebuilt Core_Root. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 99276d5-efee-49e9-beec-ac1a336d1003
Resolve the Wasm instruction enum conflict by retaining both the branch's return and load16 support and upstream's direct-call opcode support. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 99276d5-efee-49e9-beec-ac1a336d1003
Remove dead and unused Wasm thunk support identified during review, clarify the dynamic portable entrypoint layout, and update the portable-entrypoint preparation comment. Strengthen compile-time validation of the generated dispatch thunk and exercise resolved and unresolved receiver targets through the same runtime virtual callsite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 99276d5-efee-49e9-beec-ac1a336d1003
|
Azure Pipelines: Successfully started running 6 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/crossgen-contrib |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes Wasm behavior across VM fixup, JIT calling conventions, Crossgen2 codegen, and the R2R format, so a maintainer should validate subtle ABI and runtime invariants before merge.
Pull request overview
This PR enables CoreCLR browser-Wasm ReadyToRun class-virtual calls to go through the existing external-method import thunk/fixup path, and then publishes a signature-compatible vtable-dispatch portable entrypoint that forwards subsequent calls via packed vtable offsets.
Changes:
- Add a new Wasm “V” (virtual-dispatch) thunk kind and codegen for forwarding virtual calls via vtable offsets stored in a dynamically published portable entrypoint.
- Extend CoreCLR’s
ExternalMethodFixupWorker(Wasm) to publish a virtual-dispatch portable entrypoint after resolving eligible class-virtual fixups. - Update JIT / R2R call classification to route only eligible Wasm virtuals through stub-dispatch, keeping interfaces/arrays/generic-context cases on the existing
LDVIRTFTNpath, plus add targeted test coverage.
File summaries
| File | Description |
|---|---|
| src/tests/JIT/opt/virtualstubdispatch/mixed/mixed.cs | Expands the existing JIT test to exercise a single class-virtual callsite with multiple receiver types (via a no-inline wrapper). |
| src/coreclr/vm/wasm/helpers.hpp | Declares GetVirtualDispatchThunk for looking up pregenerated signature-specific vtable-dispatch thunks. |
| src/coreclr/vm/wasm/helpers.cpp | Factors portable-entrypoint thunk lookup to accept a prefix and implements GetVirtualDispatchThunk using the new V prefix. |
| src/coreclr/vm/prestub.cpp | On Wasm, publishes a READYTORUN_VIRTUAL_DISPATCH_PORTABLE_ENTRYPOINT during external method fixup for eligible class-virtual calls. |
| src/coreclr/vm/method.cpp | Updates the comment on EnsurePortableEntryPointIsCallableFromR2R to reflect its new callsite (R2R virtual dispatch fixup). |
| src/coreclr/vm/cgensys.h | Minor whitespace-only cleanup around ExternalMethodFixupWorker declaration. |
| src/coreclr/tools/Common/Compiler/ObjectWriter/WasmInstructions.cs | Adds Wasm instruction encodings/helpers needed by the virtual-dispatch thunk (i32.eqz, i32.load16_u). |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs | Refines Wasm call-kind selection to allow stub-dispatch only for eligible class-virtual calls; updates related assertions/comments. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj | Includes the new WasmVirtualDispatchThunkNode.cs in the build. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs | Adds a Wasm virtual-dispatch thunk node cache and ensures DispatchImports entry sizing supports Wasm’s needs. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmVirtualDispatchThunkNode.cs | New node that emits the signature-specific Wasm thunk that performs vtable-based portable-entrypoint dispatch and fallback. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmImportThunkPortableEntrypoint.cs | Removes the assertion that previously rejected virtual-call imports on Wasm. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmImportThunk.cs | Allows virtual-call imports on Wasm, and adds a dependency to ensure the corresponding V thunk is generated when needed. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCases/Webcil/WasmVirtualDispatch.cs | New Wasm-focused test input that creates a straightforward virtual dispatch pattern. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCases/R2RTestSuites.cs | Adds a Wasm-only R2R validation that checks thunk registration and expected instruction sequences in the emitted Wasm body. |
| src/coreclr/jit/morph.cpp | On Wasm, stops adding the VirtualStubCell hidden argument for stub-dispatch calls. |
| src/coreclr/jit/importercalls.cpp | Updates commentary clarifying that only some Wasm virtual calls route through LDVIRTFTN now. |
| src/coreclr/inc/readytorun.h | Defines READYTORUN_VIRTUAL_DISPATCH_PORTABLE_ENTRYPOINT (Wasm-only) to carry the V thunk and packed offsets. |
| docs/design/coreclr/botr/readytorun-format.md | Documents the new V prefix, call flow, and the 12-byte virtual-dispatch portable entrypoint layout. |
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 0
- Review effort level: Lite
Why is the code size growth so small? I would expect these thunks to add up to more than just 67 bytes of CODE. Also, I am wondering whether it would be a better (size) tradeoff to emit call that resolves the target followed by an actual call. We would not need target signature specific virtual dispatch thunks and we can reuse the same shape for interface calls as well. |
|
@jkotas I was actually quite surprised that there was size growth at all. This code is taking logic where we need to do 3 R2R table lookups + 2 indirect calls, into 1 R2R lookup and 1 indirect call. And in fact the size of the normal managed functions DOES shrink by about 77KB .... however, it turns out that by moving to generating virtual method dispatch like we do only other platforms, turned on detection to generate unboxing stubs for ToString, Equals, and GetHashCode implementations on structures. This caused the compiler to find a large number of unboxing stubs that it needed to generate, which is what drove all of the code byte wins to be lost + significant losses in the data size to handle having about 428 new unboxing stubs. (My measurements from before @BrzVlad merged his work to add unboxings stub support to crossgen showed that there was a 0.5% size decrease for Code size, and a slightly smaller increase for Data, yielding an overall size decrease of about 0.15% with this change). The cost of the signature specific thunks is actually very small. Currently we need 67 of them, but my analysis indicates we actually could cut THAT number in half or so by sharing the virtual thunks at the Wasm calling convention level. Also, it appears that we actually generate extra delay-load code when we don't need it. We're generating 62 duplicate copies of delay load code in this PR right now. I'm going to throw this PR back to draft to deal with the excess number of thunks we generate. In addition there are also a set of other size optimizations that I see that I found in this investigation.
|
Key Wasm import thunk code only by the properties that affect its generated body so DispatchImports can reuse matching MethodImports thunks. Move virtual-dispatch thunk rooting to the per-import portable entrypoint to preserve the dependency when code is shared.\n\nAllow non-generic virtual methods declared on generic classes to use Wasm vtable dispatch while keeping true generic virtual methods on the existing ldvirtftn path. Add generated-image assertions and runtime coverage for generic-owner dispatch and thunk sharing.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 99276d5-efee-49e9-beec-ac1a336d1003
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 99276d5-efee-49e9-beec-ac1a336d1003
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 99276d5-efee-49e9-beec-ac1a336d1003
|
Azure Pipelines: Successfully started running 6 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The WASM change to suppress the VirtualStubCell well-known arg conflicts with existing VSD lowering assumptions (asserts/incorrect lowering) and needs coordinated updates before it’s safe to merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It changes VM+JIT+crossgen2 codegen paths for Wasm R2R virtual dispatch, with cross-component correctness implications that require focused human review beyond the single confirmed issue noted.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Lite
Assert packed dispatch offsets only for 32-bit targets while retaining the wider-target fallback, and use the standard VM contract for Wasm unboxing stub lookup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 99276d5-efee-49e9-beec-ac1a336d1003
There was a problem hiding this comment.
🟡 Changes recommended
There are verified correctness/consistency issues (notably GetVirtualDispatchThunk behavior vs its contract and Wasm array-virtual call routing not matching the PR’s stated design) that should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Lite
jkotas
left a comment
There was a problem hiding this comment.
Check "WASM R2R runtime tests" for regressions before merging
Classify synthetic array methods as resolved direct calls while preserving callvirt null checks. Model the hidden array Address type argument consistently in Wasm lowering and GC ref maps, and re-enable the generic multidimensional-array smoke coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78da0c8f-d377-4b93-99ea-6e442c4a899e
There was a problem hiding this comment.
🔵 Needs a closer look
Add coverage for the generated thunk body and the unresolved-target fallback path before approval.
Review details
Suppressed comments (2)
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCases/R2RTestSuites.cs:177
- This validation only inspects import metadata and canonical thunk keys; it never inspects or executes the generated
WasmVirtualDispatchThunkNodebody. A regression in the packed-offset loads, unresolved-target fallback, or forwardedcall_indirectarguments would still pass these assertions. Please add bytecode-level checks for those instructions or an executable R2R/Wasm test that exercises the fallback path.
Validate = Validate,
},
]));
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmVirtualDispatchThunkNode.cs:149
- This fallback branch is not behaviorally exercised by the tests added here.
R2RTestRunneronly compiles and inspects the image, and the mixed test invokes targets that are compiled in the same R2R test assembly, so no test leaves a vtable targetPortableEntryPointwith a null code slot and verifies thatInitialEntryre-enters the delay-load fixup. Please add a cross-module/non-R2R target case (or equivalent setup) that forces this branch; otherwise a regression here can still pass the current tests and trap on the first call.
[!NOTE] This review comment was generated with GitHub Copilot.
// Redispatch through the original entrypoint if this receiver's target is not yet callable from R2R.
expressions.Add(Local.Get(targetCodeLocalIndex));
expressions.Add(I32.Eqz);
expressions.Add(Block.If(WasmBlockType.Empty));
expressions.Add(Local.Get(portableEntrypointLocalIndex));
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/ba-g The test failures are now only in mono which isn't affected by this change. @jkotas could you approve? |
|
it looks like this caused #134144 |
## Summary Enable ReadyToRun class-virtual calls on CoreCLR browser-Wasm to use the existing external-method import thunk and fixup pathway, then dynamically publish a signature-compatible vtable-dispatch portable entrypoint at the callsite. The first invocation enters the ordinary `WasmImportThunk` and `ExternalMethodFixupWorker`. For eligible class-virtual calls, the worker resolves the receiver's restored vtable slot, prepares the selected method for R2R calls, looks up the matching `V` thunk in the existing dynamic thunk table, allocates a virtual-dispatch portable entrypoint from the module loader allocator, and atomically publishes it. Subsequent calls load the selected target portable entrypoint through packed vtable offsets. If that target has not yet been prepared, the thunk substitutes the original import portable entrypoint and re-enters the normal external-method fixup path. Both paths share one final `call_indirect`. Wasm no longer receives the JIT's `VirtualStubCell` hidden argument. The portable entrypoint already identifies the dispatch cell, and retaining the extra argument made the managed callsite signature incompatible with the generated thunk, including in tail-call position. The generated code is shared at two levels: - Dispatch imports reuse ordinary method-import delay-load thunk code when their rich managed lowering signatures match. - Virtual-dispatch thunks are keyed only by the actual Wasm function type. Managed distinctions that lower identically, such as an indirect structure argument and an `i32` argument on Wasm32, share one `V` thunk. Non-generic virtual methods declared on generic types use this path, including calls from shared generic code whose exact context requires a runtime lookup. Interfaces, arrays, and true generic virtual methods continue to use the existing `LDVIRTFTN` path. > [!NOTE] > This pull request description was generated with GitHub Copilot. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com> Copilot-Session: 78da0c8f-d377-4b93-99ea-6e442c4a899e Copilot-Session: 99276d5-efee-49e9-beec-ac1a336d1003
…ync inliners (#134697) With cross-module inlining enabled (`--opt-cross-module`), crossgen2 can fail while emitting the cross-module inlining info table: ``` System.OverflowException: Arithmetic operation resulted in an overflow. at ILCompiler.DependencyAnalysis.ReadyToRun.InliningInfoNode.GetData(...) ``` Debug builds assert `_index != InvalidOffset` at the same place. ## Root cause When the runtime-async variant of a cross-module generic method is compiled into the image and inlines another method, `CompileMethod` records a `Check_IL_Body` fixup for the method's typical definition, which is the `AsyncMethodVariant`. `InliningInfoNode` reduced every inliner to its primary `EcmaMethod` and looked up that method's `Check_IL_Body` import instead. Nothing ever marked that import, so it was never placed, its index stayed at `int.MinValue`, and the checked `(uint)` cast threw. For an `async` method the `EcmaMethod` is the compiler-generated thunk, so debug builds instead assert while creating the signature. In the reported app, the failing inliner was the async variant of `Task<Completion<...>>.WaitAsync(CancellationToken)` (a CoreLib generic over an app type) inlining `TimeProvider.get_System()`. The cause is the async inlining change in #125472. #133146 probably just changed codegen enough to produce this case in the reported app. The bug isn't Wasm-specific: the new test reproduces it on osx-arm64. Browser CoreCLR R2R hits it first because its targets pass `--opt-cross-module:*` by default. ## Fix `CompileMethod` and `InliningInfoNode` now share `ILBodyFixupSignature.GetSignatureMethodForCompiledMethod`, which returns the typical `EcmaMethod` or `AsyncMethodVariant` that gets a compiled method's own `Check_IL_Body` fixup, or null if it can't have one. `InliningInfoNode` keeps one inliner entry per `EcmaMethod`, but stores that identity for cross-module inliners. Cross-module inliners with no such identity (async resumption stubs, return-dropping thunks, and other wrappers) or whose identity is a compiler-generated async thunk are not reported, since no `Check_IL_Body` import describes them. Inliners inside the version bubble are still encoded by `EcmaMethod` RID. When both variants inline the same method, the entry chosen doesn't depend on enumeration order. The runtime reader (`inlinetracking.cpp`, `GetILBodyTokenInfo`) uses only the module and token from these imports, so the output means the same thing. The inlining info format is unchanged. ## IL body checks now include runtime-async and synchronized flags The IL body signature used by `Check_IL_Body` and `Verify_IL_Body` covered the IL bytes, EH clauses, locals and referenced metadata, but not the method's impl flags. Both variants of a method read the same IL through the method token, so a method whose `miAsync` flag changed while its IL stayed the same still matched, and stale cross-module inlined code was used. `miSynchronized` had the same gap: a stale inline would skip the lock. `ReadyToRunStandaloneMethodMetadata` in crossgen2 and the VM now both record `miAsync` (`0x04`) and `miSynchronized` (`0x08`) in the byte that describes the locals table. Methods without these flags produce the same blob as before. The bits are documented in `readytorun-format.md`. The VM also rejects the fixup when the runtime method has no IL header, for example the task-returning variant of a method that became runtime-async, instead of reading a header that doesn't exist. If crossgen2 and the runtime come from different sides of this change, they reject each other's IL body checks for async or synchronized methods. For `Check_IL_Body` that only costs the precompiled code. `Verify_IL_Body`, used for Debug/Checked CoreLib, fails fast, so a local build that mixes a new VM with an old CoreLib image needs a CoreLib rebuild. The R2R version is unchanged, because older runtimes reject the new blob rather than misread it. ## Validation - Added `AsyncCrossModuleGenericInliner` to the R2R test suite. It fails without the fix (on osx-arm64) and passes with it. It also covers a method whose task-returning and async variants both inline the same cross-module inlinee. The full `ILCompiler.ReadyToRun.Tests` suite passes locally: 35 passed, 13 skipped for platform. - The skip for stubs and thunks has no test. I couldn't get crossgen2 to produce a return-dropping thunk that inlines something. - Replayed the crossgen2 command line from the failing `dotnet-inspect` browser CoreCLR R2R publish (`12.0.100-alpha.1.26472.116`) with a Debug crossgen2 built from this branch. It now emits `DotnetInspect.Web.Core.dll` without asserts. - Added `src/tests/readytorun/ilbody-implflags-versioning`. It compiles a consumer with `--opt-cross-module` against helper v1 and runs it against v2, where the helper methods have byte-identical IL and differ only in impl flags. The cases are a method that becomes runtime-async (awaited, and called for its `Task`), one that stops being runtime-async, and one that becomes synchronized. Without the IL body change, all three async checks fail. Running against v1 fails all four checks, including the synchronized one. With the change the test passes. The existing `readytorun/tests` (`mainv1`/`v2`/`v3`) and `async-inline-thunks` tests still pass. These were run locally on osx-arm64 Debug. Resolves #134015 > [!NOTE] > This PR description was generated with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Summary
Enable ReadyToRun class-virtual calls on CoreCLR browser-Wasm to use the existing external-method import thunk and fixup pathway, then dynamically publish a signature-compatible vtable-dispatch portable entrypoint at the callsite.
The first invocation enters the ordinary
WasmImportThunkandExternalMethodFixupWorker. For eligible class-virtual calls, the worker resolves the receiver's restored vtable slot, prepares the selected method for R2R calls, looks up the matchingVthunk in the existing dynamic thunk table, allocates a virtual-dispatch portable entrypoint from the module loader allocator, and atomically publishes it. Subsequent calls load the selected target portable entrypoint through packed vtable offsets. If that target has not yet been prepared, the thunk substitutes the original import portable entrypoint and re-enters the normal external-method fixup path. Both paths share one finalcall_indirect.Wasm no longer receives the JIT's
VirtualStubCellhidden argument. The portable entrypoint already identifies the dispatch cell, and retaining the extra argument made the managed callsite signature incompatible with the generated thunk, including in tail-call position.The generated code is shared at two levels:
i32argument on Wasm32, share oneVthunk.Non-generic virtual methods declared on generic types use this path, including calls from shared generic code whose exact context requires a runtime lookup. Interfaces, arrays, and true generic virtual methods continue to use the existing
LDVIRTFTNpath.Validation
Built the Release browser CoreCLR runtime and regenerated the browser test layout:
The focused
R2RTestSuites.WasmVirtualDispatchtest passes. It validates:Vthunk for an indirect structure argument and a pointer-sized argument;call_indirectsequences.The existing
JIT\opt\virtualstubdispatch\mixedtest was expanded to cover multiple receiver types, a virtual method on a generic owning type, and ABI-equivalent indirect-structure and pointer-sized virtual signatures. It passes under Node.js after Crossgen2 compilation:Steady-state virtual-call throughput
An ad hoc Node.js harness measured no-inline virtual calls between R2R-compiled functions. The managed DLL was identical for baseline and final; each build's Crossgen2 produced its own Wasm R2R image. Startup was excluded. Each process calibrated approximately one-second samples, performed three full warmups, forced a full GC before each measured sample, and recorded 12 samples per scenario. Three process rounds alternated baseline/final ordering, producing 36 samples per result.
Sample standard deviations were 0.32 ns baseline / 0.28 ns final for monomorphic calls and 0.09 ns baseline / 0.04 ns final for bimorphic calls.
Wasm image size
Sizes compare matched clean Release browser CoreCLR builds at the current merge base,
80a8cdbdb9c36ccab97374afcb2f0491ae58a94d, and PR tip020406010b645e209604cf0c7a183b179512a30a. Both builds used:Section sizes are Wasm section payload sizes; total is the filesystem length.
System.Private.CoreLib.wasmtotalCODEDATAnameNote
This pull request description was generated with GitHub Copilot.