-
Notifications
You must be signed in to change notification settings - Fork 127
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
Differential output tooltips are confusing #293
Comments
Alternatives:
I can try to submit a PR if you are happy with this or some other proposal. And would need to know if you care about backwards compat. My feeling is current behavior is useless enough that just removing it would be fine, but that's me. |
Perhaps should file this as a separate bug, but also it looks like maybe the scaling factor option is broken when using differential flamegraphs. Worth checking this works as part of a fix to this issue, at minimum. |
I think you're completely right that this is confusing, and I like your proposal to make it multipliers instead (and consistent (inverted?) across directions). Totally fine with removing the current info and replacing with this better info 👍 |
Thank you! Will get the relevant PR in better shape once I'm recovered from COVID. |
Consider these two compressed line profiling reports.
Baseline:
Modified:
The backwards looking difference flamegraph has explanatory tooltips as follows:
both1 (25 units, 12.50%; 0.00%)
both2 (50 units, 25.00%; +12.50%)
changed_only (125 units, 62.50%; +62.50%)
The forwards looking difference flamegraph:
base_only (50 units, 50.00%; -50.00%)
both1 (25 units, 25.00%; 0.00%)
both2 (25 units, 25.00%; +25.00%)
The main issue is the difference (+/-) percentages. They are really bad at explaining what happened.
Consider
both2
: it went from 25 to 50. In the backwards looking diff that's described as+12.50%
even though it doubled. In the forwards looking diff that's described asboth2 (25 other counts, 25.00%; +25.00%)
.+%12.50
vs+25.00%
.+0.00%
even though everything is twice as slow!Consider
changed_only
: it didn't exist in the base profile, only in the changed profile.+62.50%
does not convey that important piece of information, it just makes it seem like it grew 62% from the base.The text was updated successfully, but these errors were encountered: