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

0.7 -> 0.9 upgrade caused reduced svg fidelity #153

Closed
liaden opened this issue Nov 26, 2019 · 6 comments
Closed

0.7 -> 0.9 upgrade caused reduced svg fidelity #153

liaden opened this issue Nov 26, 2019 · 6 comments

Comments

@liaden
Copy link

liaden commented Nov 26, 2019

I noticed this on rbspy specifically: rbspy/rbspy#239
@jonhoo
Copy link
Owner

jonhoo commented Nov 26, 2019

Hmm, interesting. I suspect this is due to #136. @jasonrhansen could you take a look? @liaden could you try generating the flamegraph with min_width set to a smaller value than the default and see if that fixes the issue?

@liaden
Copy link
Author

liaden commented Nov 26, 2019

Just removing https://github.com/rbspy/rbspy/blob/master/src/ui/flamegraph.rs#L35 to take the default value, it works as expected.

@jonhoo
Copy link
Owner

jonhoo commented Nov 26, 2019 •
edited
Loading

Ah, interesting! Yes, the meaning of min_with changed in the move from 0.7 to 0.9 (I should probably have documented that in a changelog somewhere). It is now (from memory) a percentage of the image width rather than a pixel value. Should be a straightforward enough change to rbspy I think?

@liaden
Copy link
Author

liaden commented Nov 26, 2019 •
edited
Loading

Yeah, it definitely seems more appropriate to change on rbspy side.

It is not (from memory) a percentage of the image width rather than a pixel value.

Did you mean now?

@jonhoo
Copy link
Owner

jonhoo commented Nov 26, 2019

Ah, yes, sorry, now 😅

@jonhoo
Copy link
Owner

jonhoo commented Nov 26, 2019

I'll close this then since it is not an issue on the inferno side of things. Thanks for the report though!

@jonhoo jonhoo closed this as completed Nov 26, 2019
asherkin added a commit to asherkin/inferno that referenced this issue Dec 28, 2020
@asherkin
Increase default flamegraph fidelity
a69457e 
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 asherkin mentioned this issue Dec 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jonhoo @liaden