Skip to content
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

Merged
merged 15 commits into from
Oct 20, 2020

Conversation

NHDaly
Copy link
Member

@NHDaly NHDaly commented Sep 21, 2020

Add support for directly displaying FlameGraphs.jl graphs.

This makes PProf fit into the standard julia profiling infrastructure.

For example:

julia> using Profile, FlameGraphs, PProf, Serialization

julia> @profile peakflops()
1.1991880148053252e11

julia> fg = FlameGraphs.flamegraph()
Node(FlameGraphs.NodeData(ip:0x0, 0x00, 1:670))

julia> pprof(webport=11111, fg)
"profile.pb.gz"

julia> Main binary filename not available.
Serving web UI on http://localhost:11111

Screen Shot 2020-10-05 at 11 14 45 PM

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.

This makes PProf fit into the standard julia profiling infrastructure.
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
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.
@NHDaly
Copy link
Member Author

NHDaly commented Oct 6, 2020

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:

  • TODO: the span in FlameGraphs.jl is in samples, not nanoseconds. So either we need to multiply by the period to get actual nanoseonds, or switch to reporting the duration in samples, if it will let us.

Here's the only one weird difference i found when comparing data from peakflops() that went through FlameGraphs vs the data that built went directly off of Profile (it's probably easier to see if you pull up the images side-by-side):

pprof(fg):
Screen Shot 2020-10-05 at 11 21 19 PM
pprof():
Screen Shot 2020-10-05 at 11 21 27 PM

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.

@NHDaly NHDaly requested a review from vchuravy October 6, 2020 05:01
@NHDaly NHDaly marked this pull request as ready for review October 6, 2020 05:01
it seemed to just work out of the box when i reported them as `event`
`count`, just like normal.
Copy link
Member

@vchuravy vchuravy left a 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.

src/flamegraphs.jl Outdated Show resolved Hide resolved
@NHDaly
Copy link
Member Author

NHDaly commented Oct 12, 2020

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!

@vchuravy
Copy link
Member

vchuravy commented Oct 12, 2020

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.

Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also needs tests.

Project.toml Show resolved Hide resolved
@NHDaly NHDaly requested a review from vchuravy October 20, 2020 02:20
@NHDaly
Copy link
Member Author

NHDaly commented Oct 20, 2020

Okay, methinks this is good to go! :)

Thanks for the suggestions

@NHDaly
Copy link
Member Author

NHDaly commented Oct 20, 2020

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.

@vchuravy vchuravy merged commit c5594d2 into master Oct 20, 2020
@vchuravy vchuravy deleted the nhd-FlameGraphs branch October 20, 2020 03:32
NHDaly added a commit that referenced this pull request Oct 20, 2020
NHDaly added a commit to NHDaly/FlameGraphs.jl that referenced this pull request Nov 28, 2020
As of JuliaPerf/PProf.jl#25, and release v1.2.0, PProf.jl has supported displaying FlameGraphs directly, via:
```julia
using PProf
pprof(fg)
```
timholy pushed a commit to timholy/FlameGraphs.jl that referenced this pull request Nov 30, 2020
As of JuliaPerf/PProf.jl#25, and release v1.2.0, PProf.jl has supported displaying FlameGraphs directly, via:
```julia
using PProf
pprof(fg)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants