JIT: Refactor async-to-sync call optimization#124798
JIT: Refactor async-to-sync call optimization#124798jakobbotsch wants to merge 5 commits intodotnet:mainfrom
Conversation
- Introduce `getAsyncOtherVariant` JIT-EE API that returns the other async variant of an async/task-returning method - Remove the `AwaitVirtual` token kind that was used to allow the VM to tell the JIT to prefer a task-returning instead of async variant - Implement the same optimization on the JIT side instead using the `getAsyncOtherVariant`, when we notice that a call will be direct Fix dotnet#124545
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Refactors the async-to-sync call optimization by removing VM-driven “prefer task-returning for direct calls” token handling and moving the decision logic into the JIT using a new JIT↔EE API (getAsyncOtherVariant).
Changes:
- Add
ICorJitInfo::getAsyncOtherVariantAPI and plumb it through VM, JIT wrappers, NativeAOT JIT interface, and SuperPMI record/replay. - Remove
CORINFO_TOKENKIND_AwaitVirtualand simplify await token resolution to always request the async variant from the VM. - Teach the JIT importer to switch back to the synchronous Task-returning call for direct calls when the async variant is a thunk.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/jitinterface.cpp | Removes AwaitVirtual-specific resolution logic; adds EE implementation of getAsyncOtherVariant. |
| src/coreclr/jit/importer.cpp | Implements JIT-side “avoid thunk for direct calls” logic using getAsyncOtherVariant. |
| src/coreclr/jit/ee_il_dll.hpp | Replaces combine(...) helper with CORINFO_CALLINFO_FLAGS `operator |
| src/coreclr/inc/corinfo.h | Removes CORINFO_TOKENKIND_AwaitVirtual; adds ICorStaticInfo::getAsyncOtherVariant. |
| src/coreclr/inc/icorjitinfoimpl_generated.h | Declares the new EE-side virtual method override. |
| src/coreclr/inc/jiteeversionguid.h | Updates the JIT/EE interface version GUID for the API shape change. |
| src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp | Adds wrapper forwarding for getAsyncOtherVariant. |
| src/coreclr/jit/ICorJitInfo_names_generated.h | Adds name for the new API. |
| src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp | Adds SuperPMI ICJI forwarding/recording hook. |
| src/coreclr/tools/superpmi/superpmi-shim-simple/icorjitinfo_generated.cpp | Adds shim forwarding for the new API. |
| src/coreclr/tools/superpmi/superpmi-shim-counter/icorjitinfo_generated.cpp | Adds shim call counting + forwarding for the new API. |
| src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp | Records/replays getAsyncOtherVariant including variantIsThunk. |
| src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h | Adds record/replay declarations + packet ID for getAsyncOtherVariant. |
| src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp | Implements record/replay storage for getAsyncOtherVariant. |
| src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h | Adds LWM map entry for GetAsyncOtherVariant. |
| src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt | Adds new API to thunk generator inputs. |
| src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs | Removes CORINFO_TOKENKIND_AwaitVirtual from managed enum mirror. |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs | Adds unmanaged callback plumbing for getAsyncOtherVariant. |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs | Updates await-token handling; adds (currently stubbed) getAsyncOtherVariant managed implementation. |
| src/coreclr/tools/aot/jitinterface/jitinterface_generated.h | Adds callback pointer + wrapper method for getAsyncOtherVariant. |
| src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs | Removes NativeAOT scanner-side “avoid thunk for direct calls” logic. |
| src/coreclr/interpreter/compiler.cpp | Removes AwaitVirtual usage in interpreter async-call peep token resolution. |
| src/coreclr/tools/aot/ilc.slnx | Adds ILLink.CodeFixProvider project to the solution. |
| src/coreclr/tools/aot/crossgen2.slnx | Adds ILLink.CodeFixProvider project to the solution. |
| <Project Path="../../../tools/illink/src/ILLink.CodeFix/ILLink.CodeFixProvider.csproj"> | ||
| <BuildType Solution="Checked|*" Project="Debug" /> | ||
| </Project> |
There was a problem hiding this comment.
This adds ILLink.CodeFixProvider to the crossgen2 solution. This appears unrelated to the PR's described JIT/EE async-variant work; please confirm it's intentional (and if not, revert it to keep the PR focused).
| pAsyncOtherVariant = pMD->GetAsyncOtherVariant(); | ||
| } | ||
| result = (CORINFO_METHOD_HANDLE)pAsyncOtherVariant; | ||
| *variantIsThunk = pAsyncOtherVariant != NULL && pAsyncOtherVariant->IsAsyncThunkMethod(); |
There was a problem hiding this comment.
CEEInfo::getAsyncOtherVariant always returns NULL because result is never assigned, and it unconditionally dereferences variantIsThunk (nullptr is allowed by some shims). This breaks the API contract and can crash callers that pass nullptr; set result to the other variant handle (or NULL) and only write through variantIsThunk when it is non-null.
| *variantIsThunk = pAsyncOtherVariant != NULL && pAsyncOtherVariant->IsAsyncThunkMethod(); | |
| if (variantIsThunk != NULL) | |
| { | |
| *variantIsThunk = (pAsyncOtherVariant != NULL) && pAsyncOtherVariant->IsAsyncThunkMethod(); | |
| } |
| private CORINFO_METHOD_STRUCT_* getAsyncOtherVariant(CORINFO_METHOD_STRUCT_* ftn, ref bool variantIsThunk) | ||
| { | ||
| MethodDesc method = HandleToObject(ftn); | ||
| if (method.IsAsyncVariant()) |
There was a problem hiding this comment.
getAsyncOtherVariant is wired into the JIT callback table, and the JIT importer now calls it for await-pattern direct calls. Throwing NotImplementedException here will break NativeAOT/ReadyToRun compilation of async methods; implement this by mapping between the async variant and task-returning variant (and set variantIsThunk based on IsAsyncThunk()/IsAsync on the returned variant).
| var _this = GetThis(thisHandle); | ||
| try | ||
| { | ||
| return _this.getAsyncOtherVariant(ftn, ref *variantIsThunk); |
There was a problem hiding this comment.
The unmanaged entrypoint for getAsyncOtherVariant dereferences variantIsThunk (ref *variantIsThunk) without a null check. The native signature allows callers to pass nullptr (and some shims already treat it as optional), so this can AV; handle a null pointer by using a local temporary bool and only writing back when non-null.
| return _this.getAsyncOtherVariant(ftn, ref *variantIsThunk); | |
| bool localVariantIsThunk = variantIsThunk is not null && *variantIsThunk; | |
| CORINFO_METHOD_STRUCT_* result = _this.getAsyncOtherVariant(ftn, ref localVariantIsThunk); | |
| if (variantIsThunk is not null) | |
| { | |
| *variantIsThunk = localVariantIsThunk; | |
| } | |
| return result; |
| CorInfoTokenKind tokenKind = | ||
| ip[0] == CEE_CALL ? CORINFO_TOKENKIND_Await : CORINFO_TOKENKIND_AwaitVirtual; | ||
| ResolveToken(getU4LittleEndian(ip + 1), tokenKind, &m_resolvedAsyncCallToken); | ||
| ResolveToken(getU4LittleEndian(ip + 1), CORINFO_TOKENKIND_Await, &m_resolvedAsyncCallToken); |
There was a problem hiding this comment.
With CORINFO_TOKENKIND_AwaitVirtual removed, ResolveAsyncCallToken now always resolves using CORINFO_TOKENKIND_Await (even for direct CEE_CALL). Previously the VM could avoid returning an async-thunk variant for direct calls; now the interpreter may end up calling thunks unnecessarily. Consider applying the same direct-call check as the JIT importer (after getCallInfo: if callInfo.kind==CORINFO_CALL and the async variant is a thunk, fall back to resolving the regular method token).
| ResolveToken(getU4LittleEndian(ip + 1), CORINFO_TOKENKIND_Await, &m_resolvedAsyncCallToken); | |
| const uint8_t opcode = *ip; | |
| const unsigned token = getU4LittleEndian(ip + 1); | |
| if (opcode == CEE_CALL) | |
| { | |
| ResolveToken(token, CORINFO_TOKENKIND_Method, &m_resolvedAsyncCallToken); | |
| } | |
| else | |
| { | |
| ResolveToken(token, CORINFO_TOKENKIND_Await, &m_resolvedAsyncCallToken); | |
| } |
| <Project Path="../../../tools/illink/src/ILLink.CodeFix/ILLink.CodeFixProvider.csproj"> | ||
| <BuildType Solution="Checked|*" Project="Debug" /> | ||
| </Project> |
There was a problem hiding this comment.
This adds ILLink.CodeFixProvider to the ilc solution. This seems unrelated to the PR's stated JIT/EE async-variant refactor and may affect solution build time/dependencies; please confirm this is intentional and, if not, remove it from the PR.
| { | ||
| assert(callInfo.sig.isAsyncCall()); | ||
| bool isSyncCallThunk; | ||
| info.compCompHnd->getAsyncOtherVariant(callInfo.hMethod, &isSyncCallThunk); |
There was a problem hiding this comment.
The return value of getAsyncOtherVariant is not being captured or used. The code re-resolves the token with CORINFO_TOKENKIND_Method instead of using the returned method handle directly. While this may work correctly in most cases, there could be edge cases where re-resolving doesn't give the exact same specialized method descriptor, particularly in generic contexts. Consider capturing and using the return value directly to avoid the redundant token resolution and ensure correctness.
| info.compCompHnd->getAsyncOtherVariant(callInfo.hMethod, &isSyncCallThunk); | |
| CORINFO_METHOD_HANDLE asyncOtherVariant = | |
| info.compCompHnd->getAsyncOtherVariant(callInfo.hMethod, &isSyncCallThunk); | |
| if ((asyncOtherVariant != nullptr) && !isSyncCallThunk) | |
| { | |
| callInfo.hMethod = asyncOtherVariant; | |
| } |
| bool isSyncCallThunk; | ||
| info.compCompHnd->getAsyncOtherVariant(callInfo.hMethod, &isSyncCallThunk); | ||
| if (!isSyncCallThunk) |
There was a problem hiding this comment.
The variable name isSyncCallThunk is misleading. When getAsyncOtherVariant is called on an async method, it returns the sync (task-returning) variant. The bool parameter indicates whether that returned sync variant is a thunk. Consider renaming to otherVariantIsThunk or syncVariantIsThunk to make the meaning clearer.
| bool isSyncCallThunk; | |
| info.compCompHnd->getAsyncOtherVariant(callInfo.hMethod, &isSyncCallThunk); | |
| if (!isSyncCallThunk) | |
| bool syncVariantIsThunk; | |
| info.compCompHnd->getAsyncOtherVariant(callInfo.hMethod, &syncVariantIsThunk); | |
| if (!syncVariantIsThunk) |
| <Project Path="../../../tools/illink/src/ILLink.CodeFix/ILLink.CodeFixProvider.csproj"> | ||
| <BuildType Solution="Checked|*" Project="Debug" /> | ||
| </Project> |
There was a problem hiding this comment.
This change adds ILLink.CodeFix project references to the solution files. This appears unrelated to the async-to-sync call optimization that is the focus of this PR. If this is an intentional change, it should be mentioned in the PR description or done in a separate commit/PR for clarity.
getAsyncOtherVariantJIT-EE API that returns the other async variant of an async/task-returning methodAwaitVirtualtoken kind that was used to allow the VM to tell the JIT to prefer a task-returning instead of async variantgetAsyncOtherVariant, when we notice that a call will be directFix #124545