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.

Sign up for GitHub

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

Jump to bottom

Replace the custom flamegraph viewer with speedscope #100

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Replace the custom flamegraph viewer with speedscope #100

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5aa74f7
Remove old flamegraph viewer
Jul 8, 2018
0103364
Update sample.rb to be usable for outputting a flamegraph
Jul 8, 2018
7ab9f9e
Import speedscope and include instructions on how to update it
Jul 8, 2018
c64829b
Integrate into the stackprof command
Jul 8, 2018
f705bfe
Actually open the browser
Jul 8, 2018
71aa3e3
Upgrade to speedscope 0.1.2
Jul 8, 2018
ee4b6b6
Retain the original switches
Aug 2, 2018
bd69ced
Switch to a single file build
Aug 2, 2018
78ef0a9
Update to a version which does not use eval
Aug 19, 2018
213b8e6
Add missing txt file
Aug 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions bin/stackprof
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,7 @@ parser = OptionParser.new(ARGV) do |o|
o.on('--graphviz', "Graphviz output (use with dot)"){ options[:format] = :graphviz }
o.on('--node-fraction [frac]', OptionParser::DecimalNumeric, 'Drop nodes representing less than [frac] fraction of samples'){ |n| options[:node_fraction] = n }
o.on('--stackcollapse', 'stackcollapse.pl compatible output (use with stackprof-flamegraph.pl)'){ options[:format] = :stackcollapse }
o.on('--flamegraph', "timeline-flamegraph output (js)"){ options[:format] = :flamegraph }
o.on('--flamegraph-viewer [f.js]', String, "open html viewer for flamegraph output\n\n"){ |file|
puts("open file://#{File.expand_path('../../lib/stackprof/flamegraph/viewer.html', __FILE__)}?data=#{File.expand_path(file)}")
exit
}
o.on('--flamegraph', "open a viewer for the flamegraph of the given profile"){ options[:format] = :flamegraph }
Copy link
Contributor

Choose a reason for hiding this comment

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

I am personally 👎 on this options consolidation change (not that I have any real pull in the final say...)

Reason being is I have used this to integrate with other repos, and having this in two separate steps is kind of ideal when you want to just save it to a path your your choosing (currently this forces the use of Tmpdir), and open at your convenience. Allowing you to choose a dir also allows you to organize your samples, even when using the private Stackprof.print_flamegraph interface, and now that is not available.

Copy link
Contributor

Choose a reason for hiding this comment

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

To maybe meet halfway:

Could you keep the options that exist currently, but change --flamegraph-viewer to optionally accept a file, and if one isn't provided, it will run a form of the Tmpdir code you currently have and open the file.

Copy link
Collaborator Author

@jlfwong jlfwong Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback! I don't have strong opinions here, and happy to do whatever would yield the best user experience here.

Open to guidance for what the commandline flags should be here and what the semantics should be.

and having this in two separate steps is kind of ideal when you want to just save it to a path your your choosing

My understanding is that if you want to collect multiple profiles for later viewing, you can collect them as the raw stackprof output into a directory of your choosing, then run the --flamegraph command to view them. I'm not sure what you would want to do with the intermediate form here other than open them to view as a flamegraph.

The intermediate form is a JavaScript ball that isn't usable (AFAIK) from anything except the flamegraph viewer.

Copy link
Contributor

Choose a reason for hiding this comment

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

The intermediate form is a JavaScript ball that isn't usable (AFAIK) from anything except the flamegraph viewer.

I always kind of thought of the --flamegraph flag command as the "compile" step of the flamegraph, and the --flamegraph-viewer flag as the execution. The "compile" step could be slow, depending on how big your stackprof blob is (you were talking about 100MB blobs, and I know the feeling myself), you might not want to repeat that process if you are looking at data over time, or with a before and after.

Sidenote (and I am NOT suggesting doing this in this PR), in regards to the "Javascript ball" being unusable, I have wondered if it made sense to have a option flag to make it a single .html file artifact instead of javsacript file but inlining the scripts (so speedscope in this case). Seems like it would avoid the whole viewer issue, and it is what the original perl version of the flamegraph does as well by making it an SVG.

Copy link
Collaborator