-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Avoid writing jitdump code for block data #122274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0846b49
571b734
f0df6e6
bc2db43
d2af966
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -339,7 +339,7 @@ void PerfMap::LogJITCompiledMethod(MethodDesc * pMethod, PCODE pCode, size_t cod | |
| s_Current->WriteLine(line); | ||
| } | ||
|
|
||
| PAL_PerfJitDump_LogMethod((void*)pCode, codeSize, name.GetUTF8(), nullptr, nullptr); | ||
| PAL_PerfJitDump_LogMethod((void*)pCode, codeSize, name.GetUTF8(), nullptr, nullptr, /*reportCodeBlock*/true); | ||
| } | ||
| } | ||
| EX_CATCH{} EX_END_CATCH | ||
|
|
@@ -380,7 +380,7 @@ void PerfMap::LogPreCompiledMethod(MethodDesc * pMethod, PCODE pCode) | |
| if (methodRegionInfo.hotSize > 0) | ||
| { | ||
| CrstHolder ch(&(s_csPerfMap)); | ||
| PAL_PerfJitDump_LogMethod((void*)methodRegionInfo.hotStartAddress, methodRegionInfo.hotSize, name.GetUTF8(), nullptr, nullptr); | ||
| PAL_PerfJitDump_LogMethod((void*)methodRegionInfo.hotStartAddress, methodRegionInfo.hotSize, name.GetUTF8(), nullptr, nullptr, /*reportCodeBlock*/true); | ||
| } | ||
|
|
||
| if (methodRegionInfo.coldSize > 0) | ||
|
|
@@ -393,7 +393,7 @@ void PerfMap::LogPreCompiledMethod(MethodDesc * pMethod, PCODE pCode) | |
| name.Append(W("[PreJit-cold]")); | ||
| } | ||
|
|
||
| PAL_PerfJitDump_LogMethod((void*)methodRegionInfo.coldStartAddress, methodRegionInfo.coldSize, name.GetUTF8(), nullptr, nullptr); | ||
| PAL_PerfJitDump_LogMethod((void*)methodRegionInfo.coldStartAddress, methodRegionInfo.coldSize, name.GetUTF8(), nullptr, nullptr, /*reportCodeBlock*/true); | ||
| } | ||
| } | ||
| EX_CATCH{} EX_END_CATCH | ||
|
|
@@ -450,7 +450,10 @@ void PerfMap::LogStubs(const char* stubType, const char* stubOwner, PCODE pCode, | |
| s_Current->WriteLine(line); | ||
| } | ||
|
|
||
| PAL_PerfJitDump_LogMethod((void*)pCode, codeSize, name.GetUTF8(), nullptr, nullptr); | ||
| // For block-level stub allocations, the memory may be reserved but not yet committed. | ||
| // Emitting code bytes in that case can cause jitdump logging to fail, and the bytes | ||
| // are optional in the jitdump specification. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels unfortunate that we would sacrifice our code bytes. I imagine one of the major reasons someone would choose the jitdump format rather than the perfmap format is to get those code bytes. I'm wondering...
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I believe the intent of #113943 was to add configuration knobs to control the volume of stub data reported to the perf tool. I have noticed that perf inject is much faster after that change, mostly it reduces the symbolic output the perf tool needs to process. Adding @davidwrighton who might have more to add. Count of jitted*.so files for each
For cases 0 and 1, we report groups of stubs as a 'virtual group of stubs'. In that case, I felt that reporting the code bytes for these aggregated functions that we are reporting didn't seem to add value. Perf inject creates a "so" file that represents a function with code inside of it. The so's that represent block data will contain a pointer to a large block of data containing code for multiple stubs (by design with 0 and 1 configurations). While the intent is not to be sacrificing code bytes, we are sacrificing reporting of unique stub data, which in turn means that we do not have unique code data that represents the aggregated symbol. In terms of missing code data overall, a relatively small percentage will be missing code. In the test case, ~1.6% is missing code (out of ~750 jitted-*.so files created, 12 were aggregated functions). I would expect for larger apps, the percentage to decrease. In cases 2 and 3, the code data will be present.
My expectation is that if customers do not wish to have code bytes might use perfmaps and not jitdumps. With that said, I would be happy to add another config switch to disable code bytes for jitdumps. Thoughts?
I will give this try.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not actively encouraging us to do it, I just wanted to leave the door open in case you or others were aware of customer interest in jitdump without code bytes. I'm not aware of it.
I had the same perception - I don't think 113943 was trying to remove code bytes. It seemed like it was an unintended bug. This PR, as-is, would elevate the missing code from bug to by-design which I'm hoping we can avoid.
Thanks! |
||
| PAL_PerfJitDump_LogMethod((void*)pCode, codeSize, name.GetUTF8(), nullptr, nullptr, /*reportCodeBlock*/ stubAllocationType != PerfMapStubType::Block); | ||
| } | ||
| } | ||
| EX_CATCH{} EX_END_CATCH | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.