Fixup CodeFragmentHeap block allocation reporting#127298
Fixup CodeFragmentHeap block allocation reporting#127298hoyosjs wants to merge 2 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes malformed Linux jitdump JIT_CODE_LOAD records emitted for block-level stub allocations when DOTNET_PerfMapStubGranularity is 0 or 1, by avoiding emitting jitdump records that would require unreadable/uninitialized code bytes. This preserves perfmap text output while preventing jitdump parser corruption.
Changes:
- Remove the
reportCodeBlockflag fromPAL_PerfJitDump_LogMethodand always writecode_sizebytes when a jitdump record is emitted. - Skip emitting jitdump records for block-level stub allocations (still emit perfmap text entries).
- Adjust
CodeFragmentHeapstub-block reporting timing for newly allocated blocks.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/coreclr/vm/perfmap.cpp |
Stops emitting jitdump entries for block stub allocations; updates jitdump logging call sites for methods/stubs to new signature. |
src/coreclr/vm/codeman.cpp |
Moves ReportStubBlock call to after size bookkeeping for new fragment blocks. |
src/coreclr/pal/src/misc/perfjitdump.cpp |
Simplifies jitdump record emission: removes reportCodeBlock and always appends code_size bytes. |
src/coreclr/pal/inc/pal.h |
Updates the PAL API signature for PAL_PerfJitDump_LogMethod. |
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
|
fyi @tommcdon who worked on the previous iteration of fixing this |
|
I got a bit more context about the intention of the settings from David. There's a few more bugs lurking here. Going to try to get one more round of fixes. |
|
Yeah, the intention is to actually report these regions, so they can be visible in a Perf trace. I'm not sure not emitting a jitdump record is the right thing here. |
6e441b5 to
6ba8003
Compare
There was a problem hiding this comment.
Pull request overview
Adjusts CoreCLR perfmap/jitdump stub reporting so stub-block entries better reflect the committed/usable ranges and unique stub names are generated safely under concurrency, aligning with the stub granularity expectations described in the PR.
Changes:
- Make stub unique-name generation use an atomic increment instead of a plain
++. - Move
ReportStubBlockto after block splitting so the reported size matches the portion retained/used. - Update the
s_StubsMappedmember intent/comment to reflect its role as a naming counter.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/coreclr/vm/perfmap.h |
Changes stub-name counter declaration/comment. |
src/coreclr/vm/perfmap.cpp |
Switches unique stub naming to InterlockedIncrement. |
src/coreclr/vm/codeman.cpp |
Reports stub-block allocation after adjusting dwSize to avoid over-reporting the range. |
94319c9 to
f6bf674
Compare
| dwSize -= dwRemaining; | ||
| } | ||
|
|
||
| ReportStubBlock(pMem, dwSize, m_kind); |
There was a problem hiding this comment.
Could we end up double reporting blocks if we don't call AllocCodeFragmentBlock?
There was a problem hiding this comment.
you can end up grabbing it from the best fit path - that but since that got aligned down and returned during the if block right above this, that would cound as a separate block and should be reported separately
#122274 fixed up some cases where block allocations produced code, but try to do a read of uncommitted memory. This change just makes it such that block records have the right size for range identification.
After this the settings provide the following expectations: