Full color support #32
Conversation
Not yet done: - Testing & Benchmarking - Hack for chain graphs
|
That's awesome! Give me a little while to walk through the patch and make comments as I go :) |
|
clippy seems to be angry with the I am okay with splitting the function up into smaller ones. The attributes management seems like a good start ? |
|
While writing the integration test, I found one of the difference with In Are we still sure that we want a different behavior ? |
|
I added an integration test. I have another test ready for the |
…ride in a separate function
|
Ah, good catch with the integration test! I think the test we actually want is: if !func.is_empty() && func.trim().is_empty() { |
| <title>uv__async_io (2 samples, 100.00%)</title><rect x="10.0" y="245" width="1180.0" height="15.0" fill="rgb(223,84,84)" rx="2" ry="2" /> | ||
| <text x="13.00" y="255.5" >uv__async_io</text> | ||
| <title>uv__io_poll (2 samples, 100.00%)</title><rect x="10.0" y="261" width="1180.0" height="15.0" fill="rgb(223,84,84)" rx="2" ry="2" /> | ||
| <text x="13.00" y="271.5" >uv__io_poll</text> |
There was a problem hiding this comment.
I'm not sure I follow why this file had to change in this way as a result of this change?
There was a problem hiding this comment.
I actually don't know. Same command line, same output (at least visually...)
There was a problem hiding this comment.
That's pretty disturbing, because this changes locations, not just colors... Could just have been merging changes from master though I suppose.
There was a problem hiding this comment.
It actually only change the svg element layout... I thought it could be because I'm juggling between multiple flamegraph.pl, but some options (--bgcolors) are not available on the version we point at in the inferno repo, so I could not have use the wrong version.
There was a problem hiding this comment.
flamegraph.pl output does not seem stable ? I generated js-perl.svg again, and I have other diffs...
There was a problem hiding this comment.
Ohh, I wonder if this is because it uses a map to keep attributes, and iteration over the map is probably non-deterministic :/
Separately, feel free to do update the flamegraph submodule!
…full_color_support
|
@JordiChauzi are there any further TODOs for this PR now? |
I don't think so ! We can always do the following tasks in other PRs:
|
|
Looks excellent! Time to merge :D Thank you, and great work 🎉 |
Adds support for #20 and for most of #18.
Supports:
--cp(with thepalette.mapfile being compatible with theflamegraph.plversion)--hash--colors COLORSThe hack for chain graphs is not in this PR.
It requires (I think) to change the borrowed
input: &strin the frames function to an owned String, because we would need to modify some lines (the ones where we need to apply the hack). I was not so sure about making that change before any review.