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

Unfoldall support #89

Merged
merged 3 commits into from
May 13, 2016
Merged

Conversation

tjake
Copy link
Contributor

@tjake tjake commented May 11, 2016

This adds support for jvm-profiling-tools/perf-map-agent#35

You can see before/after here: https://gist.github.com/tjake/b762c51cc8a5ee89df290f2b4f361f81

Note: this patch is on top of #88

@brendangregg
Copy link
Owner

Thanks, I'll test it out. (And I still really need to make a test suite for stackcollapse-perf.pl, since changing the regexp's can be tricky; I did put some sample perf output in ~/test for manually checking). One hesitation I had was from an old bug with perf-map-agent unfolding, which Min Zhou reported:

See https://github.com/jrudolph/perf-map-agent/blob/master/perf-map-agent.c#L113
if the value of record->numpcs is zero or cur_method always equal to top_method, the entry_p will never be assigned to an valid address, thus it will be a wild pointer.
This wild pointer will be passed to fprintf, which will crash the target JVM in this case.

That was many months ago, and it might have been fixed already since then. Just something I'd want to check before encouraging people to use this. :)

@tjake
Copy link
Contributor Author

tjake commented May 11, 2016

According to jvm-profiling-tools/perf-map-agent#25 it seems to be working, and based on my tests it's doing the right thing if that means anything :)

@tjake
Copy link
Contributor Author

tjake commented May 12, 2016

another commit that adds inline annotation and fixes the coloring

https://gist.github.com/tjake/6c54350c69f86f9e1be37de7866c25d8

@brendangregg
Copy link
Owner

oh good, that's how I was going to do the inline annotation too. :) That flame graph looks great.

@brendangregg
Copy link
Owner

Tested, works, thanks! Maybe you should write a blog post about this new feature and include that awesome flame graph example.

It does increase the map file size, as expected (my prod workload went from 8 Mbytes to 250), and the SVG size too. All expected...

@brendangregg brendangregg merged commit eaed26b into brendangregg:master May 13, 2016
@jrudolph
Copy link

Very nice. Thanks @tjake for pushing this change. I hacked together something similar (on which I spent only very limited time with my very limited perl skills...) but it wasn't yet ready for a PR. This looks much nicer!

@jonhoo
Copy link

jonhoo commented Jan 29, 2019

Hi all! I'm working on a Rust port of flamegraph, and we're about to add this feature to the port (see jonhoo/inferno#13). Did you ever end up with a test case for this? I can't see any occurrences of -> in the files in test/? If not, do any of you have some perf data laying around that might serve as a test-case? It could be included in flamegraph's test suite too (which the Rust port actually uses!).

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.

4 participants