[wasm][coreclr] Cache calli cookies#127016
Conversation
Avoid repeated expensive calls to get calli cookie by caching it
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR reduces overhead in the WASM interpreter call path by caching the computed calli cookie on the MethodDesc, avoiding repeated (and potentially expensive) cookie computation for repeated invocations of the same managed method.
Changes:
- Cache and reuse a per-
MethodDesccalli cookie inInvokeManagedMethodon WASM. - Extend
MethodDescCodeData(portable entrypoints + interpreter builds) to store aCalliCookie. - Add
MethodDesc::{GetCalliCookie, SetCalliCookie}APIs to manage the cached value thread-safely.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/vm/wasm/helpers.cpp | Uses MethodDesc-cached cookie to avoid repeated GetCookieForCalliSig calls. |
| src/coreclr/vm/method.hpp | Adds CalliCookie storage in MethodDescCodeData and declares accessor APIs under the relevant feature flags. |
| src/coreclr/vm/method.cpp | Implements thread-safe cookie get/set using EnsureCodeDataExists + interlocked/volatile operations. |
…Cookie typedef - Define InterpreterCalliCookie typedef: function pointer on WASM, CallStubHeader* on non-WASM (Jan's feedback) - Unify SetCalliCookie/GetCalliCookie API, removing the separate SetCallStub/GetCallStub and FEATURE_PORTABLE_ENTRYPOINTS branching in MethodDesc - Cache calli cookie on targetMethod in the WASM calli path in interpexec.cpp (Jan's feedback) - Re-read cookie after SetCalliCookie in wasm/helpers.cpp to use the race-winning value (Aaron's feedback) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/coreclr/vm/wasm/helpers.cpp:435
CalliStubParam::cookieis now typed asInterpreterCalliCookie, but these calls still cast the cookie tovoid(*)(PCODE, int8_t*, int8_t*). IfInterpreterCalliCookiealready has that signature in wasm builds, you can invoke it directly without the cast (and it avoids extra reliance on function-pointer casts).
void InvokeCalliStub(CalliStubParam* pParam)
{
_ASSERTE(pParam->ftn != (PCODE)NULL);
_ASSERTE(pParam->cookie != NULL);
// WASM-TODO: Reconcile calling conventions for managed calli.
PCODE actualFtn = (PCODE)PortableEntryPoint::GetActualCode(pParam->ftn);
((void(*)(PCODE, int8_t*, int8_t*))pParam->cookie)(actualFtn, pParam->pArgs, pParam->pRet);
}
void InvokeUnmanagedCalli(PCODE ftn, InterpreterCalliCookie cookie, int8_t *pArgs, int8_t *pRet)
{
_ASSERTE(ftn != (PCODE)NULL);
_ASSERTE(cookie != NULL);
((void(*)(PCODE, int8_t*, int8_t*))cookie)(ftn, pArgs, pRet);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
src/coreclr/vm/method.hpp:272
MethodDescCodeDatastores the cached cookie asvoid*, but the accessor/mutator APIs are typed asInterpreterCalliCookie(which is a function pointer on WASM). This forces repeated casts between function pointers andvoid*and can trip toolchain warnings / UB concerns. Consider storing the field asInterpreterCalliCookie(or aunion/std::atomic<InterpreterCalliCookie>-style storage) soSetCalliCookie/GetCalliCookiedon’t need object-pointer casts on portable-entrypoint builds.
#endif // FEATURE_INTERPRETER
#if defined(_DEBUG) && defined(ALLOW_SXS_JIT)
PatchpointInfo *AltJitPatchpointInfo;
#endif // _DEBUG && ALLOW_SXS_JIT
src/coreclr/vm/method.cpp:290
SetCalliCookiecurrently castsInterpreterCalliCookietovoid*for the interlocked CAS. On WASMInterpreterCalliCookieis a function pointer, and function-pointer <->void*casts are non-standard and may be rejected/warned under some toolchains (especially with -Werror). If possible, makem_codeData->CalliCookiebe typed asInterpreterCalliCookieso the CAS can operate on the correctly-typed field without these casts.
IfFailRet(EnsureCodeDataExists(NULL));
_ASSERTE(m_codeData != NULL);
VolatileStoreWithoutBarrier(&m_codeData->AltJitPatchpointInfo, pInfo);
return S_OK;
}
src/coreclr/vm/wasm/helpers.cpp:598
StringToWasmSigThunkHashstores values asvoid*, butLookupThunknow uses anInterpreterCalliCookielocal and passes its address toLookupvia(void**)&thunk. This relies on type-punning a function-pointer object asvoid*storage. A safer pattern here would be to look up into avoid*temp and then cast the returned value toInterpreterCalliCookiefor the return.
}
extern "C" void RhpInterfaceDispatch8()
{
PORTABILITY_ASSERT("RhpInterfaceDispatch8 is not implemented on wasm");
| void InvokeManagedMethod(ManagedMethodParam *pParam); | ||
|
|
||
| #ifdef FEATURE_INTERPRETER | ||
| struct CalliStubParam | ||
| { | ||
| PCODE ftn; | ||
| void* cookie; | ||
| InterpreterCalliCookie cookie; | ||
| int8_t *pArgs; | ||
| int8_t *pRet; |
There was a problem hiding this comment.
CalliStubParam.cookie is now InterpreterCalliCookie, but interpexec.h doesn’t define that typedef or include the header that does (it relies on build/PCH include order). This makes the header non self-contained and can break compilation for any TU that includes interpexec.h without pulling in method.hpp first. Consider including the defining header here or moving the typedef to a header that interpexec.h already depends on under FEATURE_INTERPRETER.
Avoid repeated expensive calls to get calli cookie by caching it
Checked in HelloWorld