Cache AsyncResumptionStub instances to avoid duplicate resumption stubs in the node factory#126132
Cache AsyncResumptionStub instances to avoid duplicate resumption stubs in the node factory#126132jtschuster wants to merge 6 commits intodotnet:mainfrom
Conversation
…ss-module AsyncResumptionStub was created via 'new' per JIT compilation context. When a method was recompiled (e.g. via PrepareForCompilationRetry for cross-module inlining), a second CorInfoImpl created a second AsyncResumptionStub for the same target method. Since NodeCache uses reference equality, both entered the dependency graph as separate MethodWithGCInfo nodes. During sorting, their CompareToImpl returned 0 (they wrap the same target method), tripping the Debug.Assert(result != 0) in TypeSystemComparer.Compare(MethodDesc, MethodDesc). Fix: Add an AsyncResumptionStubHashtable (LockFreeReaderHashtable) to CompilerTypeSystemContext that interns AsyncResumptionStub instances by (targetMethod, owningType). Update all creation sites to use the new GetAsyncResumptionStub() method instead of 'new AsyncResumptionStub(...)'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a NativeAOT/ReadyToRun compilation correctness issue where multiple AsyncResumptionStub instances could be created for the same target method across recompilation contexts, causing duplicate nodes and comparer assertions during dependency graph sorting. It does so by interning resumption stubs in the type system context and updating all known creation sites to use the interned instance.
Changes:
- Added an
AsyncResumptionStubHashtabletoCompilerTypeSystemContextto cache/internAsyncResumptionStubinstances by(targetMethod, owningType). - Updated RyuJit and ReadyToRun JIT interface codepaths to use
GetAsyncResumptionStub(...)instead ofnew AsyncResumptionStub(...). - Updated the IL scanner to depend on the cached resumption stub entrypoint rather than a freshly-allocated stub.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs | Uses the context-level cached async resumption stub instead of allocating per CorInfoImpl instance. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs | Switches ReadyToRun async resumption stub creation to the cached/interned implementation and keeps fixup behavior. |
| src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs | Ensures scanner dependencies reference the interned resumption stub entrypoint for async state machines. |
| src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Async.cs | Introduces the lock-free hashtable + API (GetAsyncResumptionStub) to intern resumption stubs by key. |
MichalStrehovsky
left a comment
There was a problem hiding this comment.
The native AOT side doesn't strictly need this, but it's good to be uniform.
src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs
Outdated
Show resolved
Hide resolved
- Use HashCode.Combine for AsyncResumptionStubKey.GetHashCode and update GetKeyHashCode/GetValueHashCode for consistency - Remove unnecessary (CompilerTypeSystemContext) casts since _compilation.TypeSystemContext already returns the right type - Delete _asyncResumptionStub field from ReadyToRun CorInfoImpl (no longer needed since stubs are cached in the type system context) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stubs are cached in the type system context hashtable, so the per-compilation field is no longer needed. Also removes the now-unnecessary cleanup in common CorInfoImpl.cs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…te calls - Remove extra blank line left in CorInfoImpl.cs cleanup path - Add _resumptionStubFixupAdded bool to prevent duplicate fixups if getAsyncResumptionStub is called multiple times per method Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ResumptionStubEntryPoint is a cached factory (NodeCache) that returns the same node instance for the same input, and GetFixupBlob() already deduplicates at emission time via merge-sort + compaction. This is consistent with how AddPrecodeFixup and other fixup methods work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
OwningType is not part of the key's identity - only TargetMethod determines uniqueness. This matches the pattern used by other similar keys in the codebase (e.g. ReadyToRunGenericHelperKey). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| => TargetMethod == other.TargetMethod; | ||
|
|
||
| public override bool Equals(object obj) | ||
| => obj is AsyncResumptionStubKey other && Equals(other); | ||
|
|
||
| public override int GetHashCode() | ||
| => TargetMethod.GetHashCode(); | ||
| } | ||
|
|
||
| private sealed class AsyncResumptionStubHashtable : LockFreeReaderHashtable<AsyncResumptionStubKey, AsyncResumptionStub> | ||
| { | ||
| protected override int GetKeyHashCode(AsyncResumptionStubKey key) => key.GetHashCode(); | ||
| protected override int GetValueHashCode(AsyncResumptionStub value) => value.TargetMethod.GetHashCode(); | ||
| protected override bool CompareKeyToValue(AsyncResumptionStubKey key, AsyncResumptionStub value) | ||
| => key.TargetMethod == value.TargetMethod; | ||
| protected override bool CompareValueToValue(AsyncResumptionStub value1, AsyncResumptionStub value2) | ||
| => value1.TargetMethod == value2.TargetMethod; |
There was a problem hiding this comment.
AsyncResumptionStubKey has an OwningType field, but Equals only compares TargetMethod. This makes the key semantics ambiguous and can hide mismatched owningType requests for the same TargetMethod. Consider either incorporating OwningType into the key comparison, or documenting/renaming to make it clear interning is only by TargetMethod and asserting OwningType consistency at the call site.
| => TargetMethod == other.TargetMethod; | |
| public override bool Equals(object obj) | |
| => obj is AsyncResumptionStubKey other && Equals(other); | |
| public override int GetHashCode() | |
| => TargetMethod.GetHashCode(); | |
| } | |
| private sealed class AsyncResumptionStubHashtable : LockFreeReaderHashtable<AsyncResumptionStubKey, AsyncResumptionStub> | |
| { | |
| protected override int GetKeyHashCode(AsyncResumptionStubKey key) => key.GetHashCode(); | |
| protected override int GetValueHashCode(AsyncResumptionStub value) => value.TargetMethod.GetHashCode(); | |
| protected override bool CompareKeyToValue(AsyncResumptionStubKey key, AsyncResumptionStub value) | |
| => key.TargetMethod == value.TargetMethod; | |
| protected override bool CompareValueToValue(AsyncResumptionStub value1, AsyncResumptionStub value2) | |
| => value1.TargetMethod == value2.TargetMethod; | |
| => TargetMethod == other.TargetMethod && OwningType == other.OwningType; | |
| public override bool Equals(object obj) | |
| => obj is AsyncResumptionStubKey other && Equals(other); | |
| public override int GetHashCode() | |
| => TargetMethod.GetHashCode() ^ OwningType.GetHashCode(); | |
| } | |
| private sealed class AsyncResumptionStubHashtable : LockFreeReaderHashtable<AsyncResumptionStubKey, AsyncResumptionStub> | |
| { | |
| protected override int GetKeyHashCode(AsyncResumptionStubKey key) => key.GetHashCode(); | |
| protected override int GetValueHashCode(AsyncResumptionStub value) | |
| => value.TargetMethod.GetHashCode() ^ value.OwningType.GetHashCode(); | |
| protected override bool CompareKeyToValue(AsyncResumptionStubKey key, AsyncResumptionStub value) | |
| => key.TargetMethod == value.TargetMethod && key.OwningType == value.OwningType; | |
| protected override bool CompareValueToValue(AsyncResumptionStub value1, AsyncResumptionStub value2) | |
| => value1.TargetMethod == value2.TargetMethod && value1.OwningType == value2.OwningType; |
| => TargetMethod == other.TargetMethod; | ||
|
|
||
| public override bool Equals(object obj) | ||
| => obj is AsyncResumptionStubKey other && Equals(other); | ||
|
|
||
| public override int GetHashCode() | ||
| => TargetMethod.GetHashCode(); | ||
| } | ||
|
|
||
| private sealed class AsyncResumptionStubHashtable : LockFreeReaderHashtable<AsyncResumptionStubKey, AsyncResumptionStub> | ||
| { | ||
| protected override int GetKeyHashCode(AsyncResumptionStubKey key) => key.GetHashCode(); | ||
| protected override int GetValueHashCode(AsyncResumptionStub value) => value.TargetMethod.GetHashCode(); | ||
| protected override bool CompareKeyToValue(AsyncResumptionStubKey key, AsyncResumptionStub value) | ||
| => key.TargetMethod == value.TargetMethod; | ||
| protected override bool CompareValueToValue(AsyncResumptionStub value1, AsyncResumptionStub value2) | ||
| => value1.TargetMethod == value2.TargetMethod; |
There was a problem hiding this comment.
AsyncResumptionStubKey.GetHashCode also ignores OwningType. If OwningType is meant to participate in interning, the hash and hashtable comparisons should be updated accordingly; otherwise consider removing OwningType from the key type and passing a canonical owning type when creating the stub.
| => TargetMethod == other.TargetMethod; | |
| public override bool Equals(object obj) | |
| => obj is AsyncResumptionStubKey other && Equals(other); | |
| public override int GetHashCode() | |
| => TargetMethod.GetHashCode(); | |
| } | |
| private sealed class AsyncResumptionStubHashtable : LockFreeReaderHashtable<AsyncResumptionStubKey, AsyncResumptionStub> | |
| { | |
| protected override int GetKeyHashCode(AsyncResumptionStubKey key) => key.GetHashCode(); | |
| protected override int GetValueHashCode(AsyncResumptionStub value) => value.TargetMethod.GetHashCode(); | |
| protected override bool CompareKeyToValue(AsyncResumptionStubKey key, AsyncResumptionStub value) | |
| => key.TargetMethod == value.TargetMethod; | |
| protected override bool CompareValueToValue(AsyncResumptionStub value1, AsyncResumptionStub value2) | |
| => value1.TargetMethod == value2.TargetMethod; | |
| => TargetMethod == other.TargetMethod && OwningType == other.OwningType; | |
| public override bool Equals(object obj) | |
| => obj is AsyncResumptionStubKey other && Equals(other); | |
| public override int GetHashCode() | |
| { | |
| unchecked | |
| { | |
| int hash = TargetMethod.GetHashCode(); | |
| hash = (hash * 31) + OwningType.GetHashCode(); | |
| return hash; | |
| } | |
| } | |
| } | |
| private sealed class AsyncResumptionStubHashtable : LockFreeReaderHashtable<AsyncResumptionStubKey, AsyncResumptionStub> | |
| { | |
| protected override int GetKeyHashCode(AsyncResumptionStubKey key) => key.GetHashCode(); | |
| protected override int GetValueHashCode(AsyncResumptionStub value) | |
| { | |
| unchecked | |
| { | |
| int hash = value.TargetMethod.GetHashCode(); | |
| hash = (hash * 31) + value.OwningType.GetHashCode(); | |
| return hash; | |
| } | |
| } | |
| protected override bool CompareKeyToValue(AsyncResumptionStubKey key, AsyncResumptionStub value) | |
| => key.TargetMethod == value.TargetMethod && key.OwningType == value.OwningType; | |
| protected override bool CompareValueToValue(AsyncResumptionStub value1, AsyncResumptionStub value2) | |
| => value1.TargetMethod == value2.TargetMethod && value1.OwningType == value2.OwningType; |
AsyncResumptionStub was created via 'new' per JIT compilation context. When a method was recompiled (e.g. via PrepareForCompilationRetry for cross-module inlining), a second CorInfoImpl created a second AsyncResumptionStub for the same target method. Since NodeCache uses reference equality, both entered the dependency graph as separate MethodWithGCInfo nodes. During sorting, their CompareToImpl returned 0 (they wrap the same target method), tripping the Debug.Assert(result != 0) in TypeSystemComparer.Compare(MethodDesc, MethodDesc). This only happened once async methods are allowed to be inlined and crossgen is passed
--opt-cross-module *.Add an AsyncResumptionStubHashtable (LockFreeReaderHashtable) to CompilerTypeSystemContext that interns AsyncResumptionStub instances by (targetMethod, owningType). Update all creation sites to use the new GetAsyncResumptionStub() method instead of 'new AsyncResumptionStub(...)'.