Change the PerfMap crst into an UNSAFE_ANYMODE crst#129021
Change the PerfMap crst into an UNSAFE_ANYMODE crst#129021davidwrighton wants to merge 5 commits into
Conversation
…ore ideal for perfmap scenarios
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR adjusts CoreCLR’s perfmap logging synchronization to avoid GC-mode toggle deadlocks by switching the PerfMap lock to CRST_UNSAFE_ANYMODE, and updates VSD (virtual call stub) stub-generation paths to safely enter preemptive mode where needed when perfmap logging is enabled.
Changes:
- Switch
PerfMap’ss_csPerfMaptoCRST_UNSAFE_ANYMODEto avoid deadlock cycles involving GC-mode toggles during lock acquisition. - Update
VirtualCallStubManagerstub-generation call sites to temporarily enter preemptive GC mode when perfmap logging is enabled, and reinitialize hash probers after mode transitions. - Add a non-
FEATURE_PERFMAPstubPerfMapimplementation inperfmap.hso call sites can compile without#ifdef FEATURE_PERFMAPwrappers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/coreclr/vm/virtualcallstub.cpp | Wrap specific stub-generation paths in GCX_MAYBE_PREEMP when perfmap is enabled, and reset probers after potential invalidation. |
| src/coreclr/vm/perfmap.h | Add a stub PerfMap class for builds where FEATURE_PERFMAP is not defined. |
| src/coreclr/vm/perfmap.cpp | Initialize the PerfMap lock as CRST_UNSAFE_ANYMODE and slightly reduce time holding the lock in LogPreCompiledMethod. |
| static bool IsEnabled() | ||
| { | ||
| #ifdef DEBUG | ||
| return true; | ||
| #else | ||
| return false; | ||
| #endif | ||
| } |
Correct LogStubs contract for use on Windows
| name.Append(W("[PreJit-cold]")); | ||
| } | ||
|
|
||
| CrstHolder ch(&(s_csPerfMap)); |
There was a problem hiding this comment.
Is this just optimization making the lock narrower or its functionally important that the if() above is no longer covered by the lock?
| static bool IsEnabled() | ||
| { | ||
| #ifdef DEBUG | ||
| return true; |
There was a problem hiding this comment.
why true here? When FEATURE_PERFMAP isn't defined I wouldn't expect this to return true.
| { | ||
| LookupHolder *pLookupHolder = GenerateLookupStub(addrOfResolver, token.To_SIZE_T()); | ||
| LookupHolder *pLookupHolder; | ||
| bool reenteredCooperativeGCMode = PerfMap::IsEnabled(); |
There was a problem hiding this comment.
I'm probably not understanding something. I thought the purpose of the ANYMODE Crst is so that we would not do a coop->preemp->coop transition but the code and naming here sounds like we will do those transitions?
| LIMITED_METHOD_CONTRACT; | ||
| CONTRACTL | ||
| { | ||
| MODE_PREEMPTIVE; |
There was a problem hiding this comment.
| MODE_PREEMPTIVE; | |
| THROWS; | |
| MODE_PREEMPTIVE; |
Match the dummy implementation
| @@ -404,7 +422,7 @@ | |||
| { | |||
| LIMITED_METHOD_CONTRACT; | |||
There was a problem hiding this comment.
| LIMITED_METHOD_CONTRACT; | |
| CONTRACTL | |
| { | |
| GC_NOTRIGGER; | |
| MODE_ANY; | |
| } | |
| CONTRACTL_END; |
| LIMITED_METHOD_CONTRACT; | ||
|
|
And make sure all logic run under the crst is safe for running in such a place
Also, add logic to handle some simple cases where moving to preemptive is fairly simple. Notably, the VirtualCallStub logic needed these fixes.