Fix flamegraph display in the presence of --diff_base. #790
Conversation
Previously, the flamegraph display was broken in the presence of `--diff_base` because it did not account for the fact that some stacks could have negative values. This change fixes the handling of negative values by tracking two numbers per displayed box that covers a set of stacks: 1. `sumpos`: the sum of the absolute values of increases in the stacks. 2. `sumneg`: the sum of the absolute values of decreases in the stacks. The width of the box is proportional to `sumpos + sumneg`. In addition, if a box has an overall decrease (`sumneg > sumpos`), the fraction of the box corresponding to the decrease is shaded green. (Similarly part of the box is shaded red if the box has an overall increase.) Changed the display of legends and tooltips to clearly show the added and removed samples. E.g., the following shows that the cost of `Function` changed from `1.44s` to `1.3s` as well as the net change and the corresponding percentages. ``` 0.14s (-1.8%) │ 1.44s (18.3%) 🠆 1.3s (16.5%) │ Function ``` Documented some of the Javascript types used for flamegraph rendering.
Codecov Report
@@ Coverage Diff @@
## main #790 +/- ##
=======================================
Coverage 66.99% 66.99%
=======================================
Files 45 45
Lines 9862 9862
=======================================
Hits 6607 6607
Misses 2795 2795
Partials 460 460 |
|
Screenshot of diff view (linear search profile with diff base pointing to binary search profile)
|
Thanks for the quick review. Looking forward to Louis's comments. |
There was a problem hiding this comment.
LGTM for added code. And thanks for adding the detailed comments and explanation!
The only concern I have is that the test coverage decreases by 3%.
Not a blocker, as I can see that test of the whole file is added in #716; but it would be nice if we can include test for this behaviour with a relatively low effort.
|
Thanks.
Yes, unfortunately none of the javascript in pprof is tested. Do you have any suggestions of how we would add javascript testing? I have a vague notion that we might need to install node in some github action and use that, but I wonder who here has the experience to figure out how to set that up. Maybe something like https://github.com/browser-actions/examples ? Another alternative would be to do manually triggered tests: if pprof is invoked with a special flag, it passes that to the javascript, and the javascript code when it sees that flag runs self tests. Bit it would be better to figure out how to have real tests. Should we continue the testing discussion outside this PR, either in an issue or in an email conversation? |
Ah, that is right; giving another look to the code coverage report, it looks like the 3% of reduction is false alert: all the reductions are from go files, which I think it is probably due to versions different of the forked directory vs. the main directory.
I don't have opinions on how to set this up neither. But definitely this is not a trivial effort in the scope of this PR.
Sure! I file another Github issue about this topic, we can discuss from there and with proper priority: #791 |
Previously, the flamegraph display was broken in the presence of
--diff_basebecause it did not account for the fact that some stacks could have negative values. This change fixes the handling of negative values by tracking two numbers per displayed box that covers a set of stacks:sumpos: the sum of the absolute values of increases in the stacks.sumneg: the sum of the absolute values of decreases in the stacks.The width of the box is proportional to
sumpos + sumneg.In addition, if a box has an overall decrease (
sumneg > sumpos), the fraction of the box corresponding to the decrease is shaded green. (Similarly part of the box is shaded red if the box has an overall increase.)Changed the display of legends and tooltips to clearly show the added and removed samples. E.g., the following shows that the cost of
Functionchanged from1.44sto1.3sas well as the net change and the corresponding percentages.Documented some of the Javascript types used for flamegraph rendering.