Change the PerfMap crst into an UNSAFE_ANYMODE crst#129021
Open
davidwrighton wants to merge 1 commit into
Open
Change the PerfMap crst into an UNSAFE_ANYMODE crst#129021davidwrighton wants to merge 1 commit into
davidwrighton wants to merge 1 commit into
Conversation
…ore ideal for perfmap scenarios
Contributor
|
Tagging subscribers to this area: @agocke |
Contributor
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. |
Comment on lines
+27
to
+34
| static bool IsEnabled() | ||
| { | ||
| #ifdef DEBUG | ||
| return true; | ||
| #else | ||
| return false; | ||
| #endif | ||
| } |
Comment on lines
+55
to
+63
| static void LogStubs(const char* stubType, const char* stubOwner, PCODE pCode, size_t codeSize, PerfMapStubType stubAllocationType) | ||
| { | ||
| CONTRACTL | ||
| { | ||
| GC_NOTRIGGER; | ||
| MODE_PREEMPTIVE; | ||
| } | ||
| CONTRACTL_END; | ||
| } |
Comment on lines
2128
to
+2134
| PCODE addrOfFail = pResolveHolder->stub()->failEntryPoint(); | ||
| bool reenteredCooperativeGCMode = false; | ||
| pDispatchHolder = GenerateDispatchStub( | ||
| target, addrOfFail, objectType, token.To_SIZE_T(), &reenteredCooperativeGCMode); | ||
| bool reenteredCooperativeGCMode = PerfMap::IsEnabled(); | ||
| { | ||
| GCX_MAYBE_PREEMP(reenteredCooperativeGCMode); | ||
| pDispatchHolder = GenerateDispatchStub( | ||
| target, addrOfFail, objectType, token.To_SIZE_T(), &reenteredCooperativeGCMode); | ||
| } |
Comment on lines
2244
to
+2250
| PCODE addrOfFail = pResolveHolder->stub()->failEntryPoint(); | ||
| bool reenteredCooperativeGCMode = false; | ||
| pDispatchHolder = GenerateDispatchStub( | ||
| target, addrOfFail, objectType, token.To_SIZE_T(), &reenteredCooperativeGCMode); | ||
| bool reenteredCooperativeGCMode = PerfMap::IsEnabled(); | ||
| { | ||
| GCX_MAYBE_PREEMP(reenteredCooperativeGCMode); | ||
| pDispatchHolder = GenerateDispatchStub( | ||
| target, addrOfFail, objectType, token.To_SIZE_T(), &reenteredCooperativeGCMode); | ||
| } |
jkotas
reviewed
Jun 5, 2026
Comment on lines
+56
to
+57
| // in cooperative mode does not introduce new GC-safety issues -- the I/O was already | ||
| // performed in cooperative mode with the previous default Crst. |
Member
There was a problem hiding this comment.
Suggested change
| // in cooperative mode does not introduce new GC-safety issues -- the I/O was already | |
| // performed in cooperative mode with the previous default Crst. | |
| // in cooperative mode does not introduce GC-safety issues. Doing I/O | |
| // in cooperative mode is still less than ideal. |
The code comments should talk about the current state of the code only. It is not very helpful to talk about what the code was in the past (it can be discussed in a PR description).
jkotas
reviewed
Jun 5, 2026
| THROWS; | ||
| MODE_PREEMPTIVE; | ||
| } | ||
| CONTRACTL_END; |
Member
There was a problem hiding this comment.
The actual non-Windows implementations of these method should be updated to have the correct contracts as well. (For example, LogPreCompiledMethod is LIMITED_METHOD_CONTRACT that's not correct.
jkotas
reviewed
Jun 5, 2026
| if (reenteredCooperativeGCMode) | ||
| { | ||
| // The prober may have been invalidated by reentering cooperative GC mode, reset it | ||
| BOOL success = lookups->SetUpProber(token.To_SIZE_T(), 0, &probeL); |
Member
There was a problem hiding this comment.
Is SetUpProber that expensive compared to all other work done on this path that we cannot just do it unconditionally?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.