Navigation Menu

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 FlameGraph output #15

Merged
merged 6 commits into from Mar 24, 2015

Conversation

andreasf
Copy link
Contributor

* New, optional command line parameter: -f
* If enabled, writes folded call stacks to output file, instead
  of statistics
* Use with the FlameGraph tool: https://github.com/brendangregg/FlameGraph
@bdarnell
Copy link
Owner

bdarnell commented Dec 6, 2014

Cool.

  • Instead of -f to mean flamegraph, it's better to add --format=flamegraph or something like that, to allow room for other formats in the future.
  • The command-line mode of plop.collector is not the only one (I rarely use it myself), so the part that actually writes to a file should probably be moved to a method of Collector (instead of callers like main() reaching into stack_counts).

@xmo-odoo
Copy link
Contributor

The command-line mode of plop.collector is not the only one (I rarely use it myself), so the part that actually writes to a file should probably be moved to a method of Collector (instead of callers like main() reaching into stack_counts).

Maybe have the collector strictly just collect stack samples, and a different object would filter and format samples, with —format only configuring the latter object?

That means the collector would always collect a list of frames though (in case the formatter needs that), the default collector would be the one folding those into a counter.

And assuming the formatters don't alter the collector, this would allow multiple outputs from a single collection pass.

@andreasf
Copy link
Contributor Author

Good idea. What do you think of the changes?

* Just store stacks during collection phase, process them afterwards
* Add --max-stacks option for plop output
* Change file extension to .flame for Flamegraph output
@andreasf
Copy link
Contributor Author

I just fixed the last commit, in case you're wondering. After a semi-failed git reset the diff was referring to a hidden commit and thus rather confusing.

@xmo-odoo
Copy link
Contributor

@andreasf the ArgumentParser conversion should probably live in a separate PR (well played on -m being a switch, I would have missed that completely and tried to set up an exclusion group of some sort).

Could use a list literal for Collector.stacks, and maybe in a future PR look into preallocating it (after profiling) to avoid a possible list resizing in the middle of stack collection.

BTW if you initialise ArgumentParser with formatter_class=ArgumentDefaultsHelpFormatter it'll automatically insert the default values after the help text: https://docs.python.org/3/library/argparse.html#formatter-class

Maybe rejiggle things around in 3 different PRs, one for the ArgumentParser conversion, one for the Formatter extraction (if @bdarnell thinks those are good ideas) and the 3rd one with just the flamegraph output?

@andreasf
Copy link
Contributor Author

You're right on all points. I thought about doing a separate PR for the argparse changes, but since the last commit is from 8 months ago, I was too lazy to split it up. People over processes ;-)

@bdarnell
Copy link
Owner

Wow, this looks great. For future reference I'd have a slight preference for separate PRs for the PEP8 changes and the move to argparse, but it's not worth splitting it up.

The one thing I might change is actually the -m flag: it's intended to be like python's -m flag and take an argument. But in any case this is a big improvement over what was there before and going to go ahead and merge it; don't worry about changing -m unless you feel like it.

bdarnell added a commit that referenced this pull request Mar 24, 2015
Add support for FlameGraph output
@bdarnell bdarnell merged commit 846a692 into bdarnell:master Mar 24, 2015
@andreasf
Copy link
Contributor Author

Thanks!

@andreasf andreasf deleted the flamegraph_support branch March 24, 2015 12:48
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.

None yet

3 participants