Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
brandtbucher commented Nov 6, 2025 • edited by github-actions bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by github-actions bot
Uh oh!
There was an error while loading. Please reload this page.
… 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.
pablogsal commented Nov 8, 2025
Seems that there is either some form of a race of somehow the windows test don't trigger the GC: |
pablogsal commented Nov 8, 2025
Another posibility is that the machines are too slow and we don't even get to run under the gc somehow? |
Uh oh!
There was an error while loading. Please reload this page.
pablogsal commented Nov 8, 2025
Maybe slow_fibonacci is too slow? 😆 |
pablogsal commented Nov 8, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 e831b33Comparepablogsal commented Nov 8, 2025
I have pushed some new tests and fixes hopefully this does the trick |
brandtbucher commented Nov 12, 2025
The flakiness of these sorts of tests is... annoying. Quitting for the night. |
pablogsal commented Nov 12, 2025
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 |
pablogsal commented Nov 12, 2025
@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. |
brandtbucher commented Nov 12, 2025
I think it's an ASan-specific thing (I can reproduce locally). I'll figure out what's going on later. |
brandtbucher commented Nov 13, 2025
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. |
pablogsal commented Nov 13, 2025
Haha nice! I assume this means that you prefer to go with GC + native in this PR then, no? |
brandtbucher commented Nov 13, 2025
Yeah, I’m happy with the current state. We can beef up the native features later if they’re worth the performance hit. |
pablogsal left a comment • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
pablogsal commented Nov 17, 2025
Fixed some merge conflicts |
336366f into python:mainUh oh!
There was an error while loading. Please reload this page.
bedevere-bot commented Nov 17, 2025
|
pablogsal commented Nov 17, 2025
This is not in this PR I will take a look |
pablogsal commented Nov 18, 2025
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 <pablogsal@gmail.com>
Example flamegraph from one of the tests:
📚 Documentation preview 📚: https://cpython-previews--141108.org.readthedocs.build/