-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Enable FEATURE_PERFMAP on OSX, and update perfjitdump.cpp to work on OSX #99986
Conversation
@dotnet-policy-service agree company="Unity Technologies" |
@vuk, this is certainly interesting. One thing that I'd like to understand a bit more about - you mention that the profiler knows how to process a jitdump file. Can you confirm that the profiler can properly resolve managed code symbols for ready to run code? I looked at the screenshot, but everything I can see shows that the methods are jitted. |
Hmm. No, I don't expect that it would. Reading a bit about ready to run + profiling symbols info, it looks like ready to run will generate an adjacent Given that, until tiered compilation kicks in to specialize some methods (and generate a jitdump record), there will be no symbols for functions that are loaded as AOT. I don't expect this to be any different from any other generic CPU profiler on macOS (maybe a .NET-focused profiler will understand them?) -- e.g. I'd expect Instruments to behave the same, along with not having any info about JIT-compiled methods at all. For |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also edit the text for the PerfMapEnabled config switch to indicate OSX is also supported:
runtime/src/coreclr/inc/clrconfigvalues.h
Line 483 in e12e2fa
RETAIL_CONFIG_DWORD_INFO(EXTERNAL_PerfMapEnabled, W("PerfMapEnabled"), 0, "This flag is used on Linux to enable writing /tmp/perf-$pid.map. It is disabled by default") |
Is the default path where the perfmap is generated the same?
Good catch, done. I looked for other obvious spots to update docs, but didn't see any; the Linux performance tracing doc is tracing-speicifc instead of profiling.
Yep, |
The thing that I want to confirm is that |
This reverts commit 157a685.
I think what's happening is there are stack samples that are themselves timestamped (by If I set From comparing some traces, I think the only methods that are missing are also ones that are Is it possible that there are JIT compiled methods that execute before |
From some investigation... I see call stacks getting captured at timestamps that are 35µs, 85µs etc. before the timestamp in the jitdump record. I've filed mstange/samply#127 for this -- a fix is to just timestamp samples with the time immediately after the actual thread capture. |
With a fix for that issue I no longer get any unresolved samples with just jitdump files from coreclr. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, LGTM, but one question below.
@vvuk , thank you for tracking this down and getting it fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me except I am confused at the same place Brian is that part of the change appears to be in unreachable code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
/ba-g Infrastructure timeouts. The affected legs do not exercise the change in meaningful way, no reason for retrying. |
Thanks all for being welcoming of a random outside PR -- quite happy with how my first coreclr contribution went (as minor as it was!) :) |
We greatly appreciate the PR and look forward to the cool profiling, thanks @vvuk! |
Awesome. Thanks so much @vvuk! Appreciate the contribution! |
…OSX (dotnet#99986) * Enable FEATURE_PERFMAP on OSX, and update perfjitdump.cpp to work on OSX * Update PerfMapEnabled documentation * Enable only on OSX, and use PlatformGetCurrentThreadId * Manual mach_absolute_time calls * Use QueryPerformanceCounter * Cleaner QueryPerformanceFrequency verification * Use FEATURE_PERFMAP * Put back conditional, but all __APPLE__ * Fix logic error when disabling jitdump
The jitdump/perfmap files are a generic way of providing jit method information to profilers and other tools. On macOS, mstange/samply natively understands jitdump files, but FEATURE_PERFMAP is limited to Linux in coreclr (because linux
perf
is where all this originally came from). This PR just enables this feature to work on macOS as well. Note that samply needs to be built & installed from source; the cargo-installed release is old and has issues.With this patch, running a dotnet program with
DOTNET_PerfMapEnabled=1
set in the environment (or other way of setting PerfMapEnabled) allows something likesamply record dotnet exec Foo.dll ...
to provide mixed native/C# profile stack information on macOS, similar to what you'd get with some profilers on Windows.