-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
GH-140643: Add <native> and <GC> frames to the sampling profiler
#141108
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
Conversation
… in the sampling profiler - Introduce a new field in the GC state to store the frame that initiated garbage collection. - Update RemoteUnwinder to include options for including "<native>" and "<GC>" frames in the stack trace. - Modify the sampling profiler to accept parameters for controlling the inclusion of native and GC frames. - Enhance the stack collector to properly format and append these frames during profiling. - Add tests to verify the correct behavior of the profiler with respect to native and GC frames, including options to exclude them.
|
Seems that there is either some form of a race of somehow the windows test don't trigger the GC: |
|
Another posibility is that the machines are too slow and we don't even get to run under the gc somehow? |
|
Maybe slow_fibonacci is too slow? 😆 |
|
I am thinking that Another idea is that maybe there is a C function in the stack maybe in another PR we can fetch the C function name and use that as the code? |
4f09326 to
e831b33
Compare
|
I have pushed some new tests and fixes hopefully this does the trick |
|
The flakiness of these sorts of tests is... annoying. Quitting for the night. |
I feel you. Unfortunately it's very hard to write correct code here as its fundamentally a race condition between the function being profiled and the profiler. Specially in slow machines it's a pain. I recommend doing one thing and one thing only per test |
|
@brandtbucher a suggestion if you struggled with CI it's to just add the GC switch in this PR and figure out native mode layer as that is currently less useful and it's giving us trouble. |
|
I think it's an ASan-specific thing (I can reproduce locally). I'll figure out what's going on later. |
|
I thought I was being clever when I also added support for native frames at the very top of the stack in a recent commit, but that only works on debug builds (where we clear the stack pointer upon resuming a Python frame). 🤦🏼♂️ Reverting, this version only finds native frames in the middle of the stack now. |
Haha nice! I assume this means that you prefer to go with GC + native in this PR then, no? |
|
Yeah, I’m happy with the current state. We can beef up the native features later if they’re worth the performance hit. |
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.
LGMT! Amazing work 💪
Left some small comments
|
Fixed some merge conflicts |
|
|
This is not in this PR I will take a look |
|
Fixing in #141688 (comment) |
…filer (python#141108) - Introduce a new field in the GC state to store the frame that initiated garbage collection. - Update RemoteUnwinder to include options for including "<native>" and "<GC>" frames in the stack trace. - Modify the sampling profiler to accept parameters for controlling the inclusion of native and GC frames. - Enhance the stack collector to properly format and append these frames during profiling. - Add tests to verify the correct behavior of the profiler with respect to native and GC frames, including options to exclude them. Co-authored-by: Pablo Galindo Salgado <[email protected]>
Example flamegraph from one of the tests:
📚 Documentation preview 📚: https://cpython-previews--141108.org.readthedocs.build/