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

Color diffusion mode #165

Merged
merged 7 commits into from
Mar 18, 2020
Merged

Color diffusion mode #165

merged 7 commits into from
Mar 18, 2020

Conversation

itamarst
Copy link
Contributor

@itamarst itamarst commented Mar 8, 2020

This does a few things:

  1. Makes differential mode less saturated red and blue, so it's easier to read.
  2. Adds a new color mode, tentatively called "color-diffusion."

The idea is that I want to visually draw the viewer's eyes towards the wider frames. Much like a differential flamegraph pulls you towards the places where things got worse (or better) with red (or blue), this highlights the bad bits in a normal flamegraph, i.e. the wider the frame the redder it gets.

So some questions for you:

  1. Do you have a better name?
  2. Are you willing to accept this? The alternative is some sort of plugable color generation API, but it would have to take a lot of inputs to cover all cases.

@jasonrhansen
Copy link
Collaborator

I personally think the color diffusion mode is a nice addition.

I also like the look of the less saturated differential flamegraphs. It makes the text more readable when the sample diffs are larger. On the other hand, I think it makes it harder to see the paler hues. If the number of samples only differs by a small amount, it almost looks like white. At least on my monitor, in the second image below I can barely see the lightest red and blue.

Before change
image

After change
image

@jonhoo
Copy link
Owner

jonhoo commented Mar 9, 2020

It would be really good to see some more before/after pictures to judge the effect of this. In general I'm for the change though.

@codecov
Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #165 into master will increase coverage by 1.33%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
+ Coverage    88.5%   89.84%   +1.33%     
==========================================
  Files          16       16              
  Lines        2228     2226       -2     
==========================================
+ Hits         1972     2000      +28     
+ Misses        256      226      -30
Impacted Files Coverage Δ
src/flamegraph/color/mod.rs 82.97% <100%> (+9.92%) ⬆️
src/flamegraph/mod.rs 95.8% <100%> (+0.04%) ⬆️
src/collapse/perf.rs 97.39% <0%> (ø) ⬆️
src/flamegraph/color/palettes.rs 99.63% <0%> (+0.36%) ⬆️
src/flamegraph/svg.rs 97.88% <0%> (+0.7%) ⬆️
src/flamegraph/color/palette_map.rs 93.8% <0%> (+0.88%) ⬆️
src/flamegraph/attrs.rs 92.92% <0%> (+1.01%) ⬆️
src/collapse/sample.rs 92.85% <0%> (+1.02%) ⬆️
src/collapse/vtune.rs 91.66% <0%> (+1.38%) ⬆️
... and 4 more

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 2aeab32...090d694. Read the comment docs.

@itamarst
Copy link
Contributor Author

Looks like I can get some more example data from https://github.com/corpaul/flamegraphdiff/tree/master/demos, let me see what they look like.

@itamarst
Copy link
Contributor Author

Nope, looks like artificial dataset, or at least just one or two function tweaked.

@itamarst
Copy link
Contributor Author

OK, here is a more realistic example, an optimization I just tried for my Python memory profiler (yes, I'm generating flamegraphs for my flamegraph-generating project 😁 ).

Now, you might say "where did all the blue go!" but if you mouse over the you'll see it's like 0.20% improvement and other similarly tiny changes. 0.20% improvement is mostly noise, so having it be less visible is fine. And maybe it should be slightly more blue.

comparison.zip

@jonhoo
Copy link
Owner

jonhoo commented Mar 16, 2020

I think slightly more contrast might be good, but apart from that I'm happy with it :)

src/flamegraph/mod.rs Outdated Show resolved Hide resolved
jonhoo
jonhoo previously approved these changes Mar 16, 2020
@jonhoo jonhoo merged commit 28fe30e into jonhoo:master Mar 18, 2020
@jonhoo
Copy link
Owner

jonhoo commented Mar 18, 2020

Beautiful, thank you!

@jonhoo
Copy link
Owner

jonhoo commented Mar 18, 2020

Ah, could you also add an entry to the "Added" section under "Unreleased" in CHANGELOG.md briefly describing the change and with a link to the PR?

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.

3 participants