-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add support for directly displaying FlameGraphs.jl graphs. #25
Conversation
This makes PProf fit into the standard julia profiling infrastructure.
c067a71
to
79f878d
Compare
Handle case where input FlameGraph data contains 0x0 pointers Use a fallback which is more robust: hash the whole StackFrame object if it doesn't have a valid Instruction Pointer and/or linfo MethodInstance.
…oo"`) Manually escape the strings before writing them to the Proto.
… `var"#foo"`)" This reverts commit e3f055c. Undo this, and fix it in upstream `google/pprof` instead
…em (e.g. `var"#foo"`)"" This reverts commit 7ec85c2.
Before, starting from leaves up, we would miss the exclusive time for parent frames that take longer than their children. Now, we walk from top-down, and record a span in these four cases: - If the node is a leaf - If there is a gap _before_ first child - If there is a gap _between_ children - If there is a gap _after_ the last child Then for each span, we walk *back* up the tree (😅) and record the stack trace. We could maybe be algorithmically more efficient with some thought? We could certainly be a constant factor more efficient. My main concern is walking down each node and then back up each stack, this might be O(n*h) where h is the height of the tree. Probably would be better to build this up as we go? But 🤷 we have to emit the whole stack every time, so I don't think there's any avoiding O(n*h) time.
3d715b1
to
7811cbc
Compare
Also, after my last rewrite, i have checked and confirmed that it does indeed produce almost exact same results whether building directly from Profile or going through FlameGraphs - so maybe we want to simplify our code and always go through FlameGraphs? I'm not sure! I kind of like having to separate implementations to validate in case either one gets an error, but ALSO i feel like FlameGraphs has many more eyes on it, and is therefore much more vetted 😅 I guess there are some inefficiencies of going through FlameGraphs, though, since we have to essentially deconstruct the aggregated view back into the samples view, so that's a bit disappointing. Oh, and also we lose individual samples, and can only report a span. This reminds me:
Here's the only one weird difference i found when comparing data from I'm not sure if this is another instance of us incorrectly duplicating a frame or something weird? But when we build off of Profile directly we duplicate the next frame down, instead of having the correct frame there. |
it seemed to just work out of the box when i reported them as `event` `count`, just like normal.
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 don't really have sufficient experience with FlameGraphs.jl. It's a shame that you have to "unprocess" the graph. So I would definitly have both versions.
Yeah, agreed. But it does seem like this contains all of the same information we need, so it might be worth it to go this hop through FlameGraphs just to have a single, consistent transformation from Profile, which gets more eyes? 🤷 Also, I think the issue I showed in the images above is the only remaining issue before this is ready to merge! |
I totally understand wanting to render FlameGraph data, especially since you can use it for other data sources. In the long run I always wanted to include the JIT_PROFILE output for perf, and for that one likely would need to go from the raw form with the instruction pointers. |
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.
Also needs tests.
Okay, methinks this is good to go! :) Thanks for the suggestions |
After looking at the above discrepancy in the two images I posted, I think the FlameGraphs image is more likely to be right, and the PProf one is simply a dropped frame due to the inlining bug. It's maybe just that i ran this on an old commit (maybe on this branch) without merging in #27. |
As of JuliaPerf/PProf.jl#25, and release v1.2.0, PProf.jl has supported displaying FlameGraphs directly, via: ```julia using PProf pprof(fg) ```
As of JuliaPerf/PProf.jl#25, and release v1.2.0, PProf.jl has supported displaying FlameGraphs directly, via: ```julia using PProf pprof(fg) ```
Add support for directly displaying FlameGraphs.jl graphs.
This makes PProf fit into the standard julia profiling infrastructure.
For example:
Unfortunately, PProf doesn't retain the ordering information that the FlameGraphs have, so it's not as good of a format for FlameGraphs directly.
But the graph view and source views are still useful, even when building from FlameGraphs.