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

Differential output tooltips are confusing #293

Open
itamarst opened this issue May 26, 2023 · 4 comments · May be fixed by #294
Open

Differential output tooltips are confusing #293

itamarst opened this issue May 26, 2023 · 4 comments · May be fixed by #294

Comments

@itamarst
Copy link
Contributor

itamarst commented May 26, 2023

Consider these two compressed line profiling reports.

Baseline:

base_only 50
both1 25
both2 25

Modified:

both1 25
both2 50
changed_only 125

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 as both2 (25 other counts, 25.00%; +25.00%).

  1. This is inconsistent: +%12.50 vs +25.00%.
  2. It's talking about % of overall time, basically, but that's not what most people care about, they care about absolute change. "This function got faster" or "this function uses more memory", not "this function is higher percentage of the whole". If everything in your program somehow ran twice as slow, it'd be +0.00% even though everything is twice as slow!
  3. If % of total is really what you care about that's why there's a normalization option when creating the diff.

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.

@itamarst
Copy link
Contributor Author

Alternatives:

  • A reasonable semantic to me is "multiplier from old to new, consistent across both directions". So in example above base2 would be new = 2.0×old (↑) in both backwards and forwards looking graphs. Something added in new would be new = added (↑) in backwards looking, something removed in new would be new = removed (↓), something shrunken in new would be new = 0.75×old (↓).
  • Perhaps you have some ideas.

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.

@itamarst itamarst linked a pull request May 26, 2023 that will close this issue
2 tasks
@itamarst
Copy link
Contributor Author

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.

@jonhoo
Copy link
Owner

jonhoo commented Aug 6, 2023

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 👍

@itamarst
Copy link
Contributor Author

itamarst commented Aug 6, 2023

Thank you! Will get the relevant PR in better shape once I'm recovered from COVID.

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 a pull request may close this issue.

2 participants