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

Replace original FlameGraph with Rust port #211

Merged
merged 5 commits into from May 20, 2019

Conversation

Projects
None yet
4 participants
@neosilky
Copy link
Collaborator

commented May 17, 2019

Inferno is a Rust port of the original FlameGraph, mainly focussing on performance.

The conversion to using Inferno was super easy and provides the same functionality. This would get rid of the requirement for perl to be installed and means we can get rid of the vendored code.

However, there currently seems to be a bug where an extra stack line is added. I'm not sure why but I'm looking into it when I get a chance.

@benfred

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2019

@neosilky - nice work on this! your change inspired me to try to do the same with py-spy.

I figured out that the extra stack line is because of a trailing ';' when building up the frame (this line on the final iteration) - which seems to make a difference with inferno but not so much with the vendored flamegraph code.

I have a PR with py-spy benfred/py-spy#114 that fixes this - and also doesn't write out a temporary file (using inferno from_lines to create). Feel free to copy anything you find useful from it, the flamegraph code in py-spy was copied from rbspy originally so should be basically the same. Eventually I'd like to merge these into a common crate (when I some more free time)

@jonhoo

This comment has been minimized.

Copy link

commented May 19, 2019

It should also be pretty easy to make inferno ignore a trailing ; -- I'd be happy to review a PR :)

@neosilky

This comment has been minimized.

Copy link
Collaborator Author

commented May 19, 2019

@benfred Nice, thanks! Will update my code when I get a chance :)

@neosilky

This comment has been minimized.

Copy link
Collaborator Author

commented May 19, 2019

Now I'm getting bad coloring 😁

image

@jvns

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2019

This is great, will merge once the coloring issue is sorted out :)

@jonhoo

This comment has been minimized.

Copy link

commented May 19, 2019

That coloring looks like you are rendering a differential flame graph. Inferno should only be generating that if you give it folded (i.e., post-stackcollapse) files that have two sample counts at the end of each line. Is that the case?

@neosilky neosilky force-pushed the neosilky:flamegraph-inferno branch from 5e5f53b to b7accb4 May 19, 2019

@neosilky

This comment has been minimized.

Copy link
Collaborator Author

commented May 19, 2019

@jonhoo Ah, I switched to joining each frame by a ; but now the end of the output is like ...;<c function> - unknown line 0 1, which looks like the differential flamegraph format you mention.

So maybe the easiest thing here is to get Inferno to ignore final ;s, or I can change the format of the line.

@neosilky

This comment has been minimized.

Copy link
Collaborator Author

commented May 19, 2019

(Woo, pure Rust!)

image

@jvns

This comment has been minimized.

Copy link
Collaborator

commented May 20, 2019

this looks great! are there more changes you want to make before merging?

@neosilky

This comment has been minimized.

Copy link
Collaborator Author

commented May 20, 2019

@jvns Should be good to go once these tests pass :)

@jvns jvns merged commit 18d4779 into rbspy:master May 20, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@neosilky neosilky deleted the neosilky:flamegraph-inferno branch May 20, 2019

@jonhoo

This comment has been minimized.

Copy link

commented May 20, 2019

@neosilky ah, I see, you specifically have a "function" in the stack trace that contains spaces, is last, and ends with a number -- yeah, that would cause some oddity. I guess the trailing ; is what saved you from that parsing ambiguity in the old version. Adding this to inferno seems like a good idea regardless, so happy to add this from a PR :)

@neosilky neosilky changed the title WIP: Replace original FlameGraph with Rust port Replace original FlameGraph with Rust port May 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.