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

Fix savings calculation. #796

Merged
merged 7 commits into from
May 21, 2024
Merged

Fix savings calculation. #796

merged 7 commits into from
May 21, 2024

Conversation

mcourteaux
Copy link
Contributor

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
Copy link
Owner

wolfpld commented May 21, 2024

Can you show how the negative case will look like?

@mcourteaux
Copy link
Contributor Author

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
Copy link
Owner

wolfpld commented May 21, 2024

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
Copy link
Contributor Author

Okay, I'll update PR soon.

@mcourteaux
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

wolfpld commented May 21, 2024

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
Copy link
Contributor Author

mcourteaux commented May 21, 2024

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
Copy link
Contributor Author

mcourteaux commented May 21, 2024

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
Copy link
Owner

wolfpld commented May 21, 2024

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

@mcourteaux
Copy link
Contributor Author

mcourteaux commented May 21, 2024

"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
Copy link
Contributor Author

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

image

@mcourteaux
Copy link
Contributor Author

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

image

@wolfpld
Copy link
Owner

wolfpld commented May 21, 2024

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
Copy link
Contributor Author

mcourteaux commented May 21, 2024

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 wolfpld merged commit 60ae46a into wolfpld:master May 21, 2024
4 checks passed
@wolfpld
Copy link
Owner

wolfpld commented May 21, 2024

I did some final adjustments in cf23441.

obraz

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 this pull request may close these issues.

2 participants