wolfpld / tracy

Frame profiler
https://tracy.nereid.pl/
Other
8.61k stars 592 forks source link

Fix savings calculation. #796

Closed mcourteaux closed 1 month ago

mcourteaux commented 1 month ago

The fraction printed represented how much of the time was left after the savings are deduced. However, as it's printed after the "Savings:" label, it makes more sense to report how much time of the original one was saved.

Going from 1s to 100ms now reports 90% savings, instead of 10%.

wolfpld commented 1 month ago

Can you show how the negative case will look like?

mcourteaux commented 1 month ago

Reduction: image

Slow down: image

Not very intuitive indeed. In this case, this would mean that we have 3.59 times slower (x3.59 == +2.59 == +259%). Perhaps we should make the displayed information conditional on whether it's a slowdown or a speedup?

if (faster) {
  // talk about savings: time saved + fraction of original reduced + speedup factor
} else {
  // talk about slowdown: time lost + slowdown factor
}

Opinions?

wolfpld commented 1 month ago

It may be worth to check how it looks. I would additionally color-code the negative result (eg., with a red text), to make it obvious that this isn't a speedup.

mcourteaux commented 1 month ago

Okay, I'll update PR soon.

mcourteaux commented 1 month ago

New version, with speedup: image

And a slowdown: image

I do like this, however, now that both the total time and the mean time are displayed with speedup/slowdown, I would argue that the normalization feature should be turned off. Thoughts?

mcourteaux commented 1 month ago

Hmm, now reporting the absolute value of the change, to get rid of the confusion - sign in the front when dealing with a slowdown:

image

wolfpld commented 1 month ago

Both slowdown labels should include a reference to what they are describing (ie., total / mean).

Normalization is useful when you can't have two traces with exactly the same number of function calls. The user has to consciously enable this option and is well informed about what will happen if they do.

I'm not sure the faster/slower reading is clear. For example, does 1.2x faster mean that the code runs 120% or 20% faster compared to the baseline? The value that was originally displayed was pretty clear in this case, even if it was mislabeled. It should read "execution time of the new code, relative to the old" instead of "savings". But that's quite a mouthful.

Some AI-generated proposals for a shorter label, Bing:

  1. New vs Old Exec Time
  2. Code Time Comparison
  3. Relative Code Speed
  4. Exec Time Ratio
  5. New/Old Time
  6. Code Speed Delta
  7. Time Efficiency
  8. Performance Shift
  9. Speed Factor
  10. Time Contrast

Llama3:

  1. New/Old Time
  2. Time Ratio
  3. Rel. Exec. Time
  4. New vs Old Time
  5. Time Delta
  6. Exec. Time Rel.
  7. Time Diff
  8. New/Old Ratio
  9. Time Change
  10. Time Rel.

I can't say that I particularly like any of these suggestions.

120% or 20% faster compared to the baseline

Maybe that's the answer. "10% of the baseline", or "of the original time" for your 1 s vs 100 ms example?

mcourteaux commented 1 month ago

Both slowdown labels should include a reference to what they are describing (ie., total / mean).

Agreed. Will fix.

Normalization is useful when you can't have two traces with exactly the same number of function calls. The user has to consciously enable this option and is well informed about what will happen if they do.

Yes, but now it's turned on by default.

I'm not sure the faster/slower reading is clear. For example, does 1.2x faster mean that the code runs 120% or 20% faster compared to the baseline? The value that was originally displayed was pretty clear in this case, even if it was mislabeled. It should read "execution time of the new code, relative to the old" instead of "savings". But that's quite a mouthful.

What about 1.430x as fast as external and 0.130x as fast as external? That's pretty unambiguous I think, and it's multiplicative (my personal preference). Alternatively: 43% faster than external and 87% slower than external. Also unambiguous, but additive.

mcourteaux commented 1 month ago

What about this?

image

I like this, as it doesn't talk about fast or slow. Because that interpretation of faster/slower can not always be made (indeed, for example, when looking at total time, and the number of runs is not equal).

wolfpld commented 1 month ago

"x% of external" would work for me. It's short and pretty clear: "10% of external" or "250% of external".

mcourteaux commented 1 month ago

"x% of external" would work for me. It's short and pretty clear: "10% of external" or "250% of external".

Yes, I like this:

image

I had to be explicit with saying "this mean time is .... of external mean time" because otherwise, it sounded like the difference is what is expressed as percentage of external.

mcourteaux commented 1 month ago

I actually think I like this less verbose, more symbolic test better:

image

mcourteaux commented 1 month ago

Not gonna lie, simplicity wins in my opinion. Sorry for the spam. I'll push this:

image

wolfpld commented 1 month ago

Can you color the icons?

Also, can you try a more subtle approach, where only the "less" or "more" is colored in the text, instead of the whole line?

mcourteaux commented 1 month ago

image

I like it! If there is anything else, it'll have to wait, as I have to go. So feel free to merge after my push if you like it!

wolfpld commented 1 month ago

I did some final adjustments in cf234411.

obraz