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

Increase default flamegraph fidelity #204

Merged
merged 2 commits into from
Dec 30, 2020
Merged

Conversation

asherkin
Copy link
Contributor

See also #153. PR #136 changed the semantics of the --minwidth option from output pixel width to percentage width (which is effectively the same as pct of total samples), which greatly reduced the default fidelity of flamegraphs as 0.1% of total time is quite coarse, especially once you start zooming in.

This PR reduces the default to 0.01%, which produces pretty much identical output as flamegraph.pl / old inferno at ~1200px wide and includes a much greater number of bars. I think this change is worth making as it makes it reduces friction for people migrating from flamegraph.pl, and in my tests produces much more useful output by default.

I've also updated the description of the option to match the current behaviour, as it still mentioned pixels and that had me confused for a good while.

See also jonhoo#153. PR jonhoo#136 changed the semantics of the `--minwidth` option
from output pixel width to percentage width (which is effectively the
same as pct of total samples), which greatly reduced the default
fidelity of flamegraphs as 0.1% of total time is quite coarse,
especially once you start zooming in.

This PR reduces the default to 0.01%, which produces pretty much
identical output as flamegraph.pl / old inferno at ~1200px wide and
includes a much greater number of bars. I think this change is worth
making as it makes it reduces friction for people migrating from
flamegraph.pl, and in my tests produces much more useful output by
default.

I've also updated the description of the option to match the current
behaviour, as it still mentioned pixels and that had me confused for
a good while.
@asherkin
Copy link
Contributor Author

flamegraph.pl --colors annotated --hash:

inferno-flamegraph --colors annotated --hash --minwidth 0.01 --width 1200: (i.e. default after this change)

inferno-flamegraph --colors annotated --hash --minwidth 0.1 --width 1200 (i.e. default before this)

I do miss the rounded corners, I think they helped pick out the edges of samples quite a bit.

@asherkin
Copy link
Contributor Author

Looks like a bunch of the test output files would need updating due to this -- I'll do that if you're happy taking this change, else just pare this down to the description fix.

@jonhoo
Copy link
Owner

jonhoo commented Dec 28, 2020

I quite like the change! Rounded corners shouldn't be too hard to add back in with CSS if you want them. I think border-radius is all you need?

@codecov
Copy link

codecov bot commented Dec 28, 2020

Codecov Report

Merging #204 (8fd0289) into master (2e07925) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #204   +/-   ##
=======================================
  Coverage   90.08%   90.08%           
=======================================
  Files          17       17           
  Lines        2239     2239           
=======================================
  Hits         2017     2017           
  Misses        222      222           
Impacted Files Coverage Δ
src/flamegraph/mod.rs 96.45% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e07925...8fd0289. Read the comment docs.

@jonhoo jonhoo merged commit a694f88 into jonhoo:master Dec 30, 2020
@asherkin asherkin deleted the min-width branch February 26, 2021 23:22
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