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

More understandable ips comparison #49

Closed

Conversation

bquorning
Copy link
Contributor

When two pieces of code run the same number of iterations in the same amount of time, that shouldn't be reported as one being "1.00x slower" than the other. By comparing all slower results against the fastest result, and not the other way around, we get some more realistic reporting.

@evanphx
Copy link
Owner

evanphx commented Aug 11, 2015

Good call on it not being clear. I worry about changing the number displayed there only because it might confuse existing users, which seem to have understood my intention despite my bad wording. Perhaps we leave the number the same and just remove he word "slower"?

@schneems
Copy link
Contributor

👍 I always do (faster - slower) / faster and never use the x.compare! feature.

If you're worried about backwards compat, we could preserve an option call it legacy_compare!, release a minor version with deprecations around compare! telling people it will change and then a major version bump with the actual change.

@evanphx
Copy link
Owner

evanphx commented Aug 11, 2015

@schneems ok! I'm happy to display more info as well, to try and make it clearer. So we could show the current value as well as a subtracted one. I want the feature to be clear, so whatever we have to do to make that possible, we should do it.

@schneems
Copy link
Contributor

How about something like this?

          addition2: 24011974.8 i/s - Fastest
          addition3: 23958619.8 i/s - 0.22 % slower (1.00x)
addition-test-long-label:  5014756.0 i/s - 79.06 % slower (4.79x)
            addition:  4955278.9 i/s - 79.317 % slower (4.85x)

@bquorning
Copy link
Contributor Author

I was considering doing the comparisons against the slowest run, and showing how many percent faster the other runs were:

          addition2: 24011974.8 i/s - 384.6 % faster
          addition3: 23958619.8 i/s - 383.5 % faster
addition-test-long-label:  5014756.0 i/s - 1.2 % faster
            addition:  4955278.9 i/s - Slowest

It seems to me that when doing performance work, you want to know how much faster you can make a piece of code. Not necessarily how much slower the old code was. Correct me if I’m wrong.

@schneems
Copy link
Contributor

I like that, I'm usually looking for speed improvements. Not sure about others though.

@bquorning bquorning force-pushed the more-understandable-ips-comparison branch from da37d1f to 458f6c5 Compare August 11, 2015 19:35
@kbrock
Copy link
Contributor

kbrock commented Aug 11, 2015

Thanks.

Maybe related or maybe not.

Sometimes, 2 different methods are about the same speed. standard deviation may be able to tell us this.

What algorithm can we use to have them say "basically the same"?

@evanphx
Copy link
Owner

evanphx commented Aug 11, 2015

Use the stddev. If the difference is within the the stddev, they are the same speed.

On Tue, Aug 11, 2015 at 1:30 PM, Keenan Brock notifications@github.com
wrote:

Thanks.
Maybe related or maybe not.
Sometimes, 2 different methods are about the same speed. standard deviation may be able to tell us this.

What algorithm can we use to have them say "basically the same"?

Reply to this email directly or view it on GitHub:
#49 (comment)

@evanphx
Copy link
Owner

evanphx commented Aug 12, 2015

I'd prefer to keep it showing the fastest and then how much slower the others are. Change that would be definitely confusing to existing users.

@kbrock
Copy link
Contributor

kbrock commented Aug 12, 2015

I had looked into including the std dev in the calculations. So I could see if some were the same. But had trouble making the information clear when showing a range.

Almost feel like it could be:
((slower - slower.stdev) - (fast + fast.stddev)) / (fast + fast.stddev). And then swap the signs for the stddev padding.

@bquorning
Copy link
Contributor Author

I would like to move the discussion back to the main topic again:

The current x = best.ips.to_f / report.ips.to_f calculation means “The fastest time was x times faster than this one (except, you should subtract 1x)”. I understand that changing the existing reporting might confuse existing users, but the current implementation apparently does also confuse existing users.

I think that it would be most confusing for existing users to only slightly change the existing output. Keeping the _.__x slower output, but giving the number a different meaning – that would be confusing. (That is what was originally proposed by this PR, and I was wrong.)

Less confusing would be to completely change the output. I think changing it to either of the proposed _.__ % slower or _.__ % faster would be less confusing for existing users.

Since it has already been mentioned that such changes might cause a major version bump: I’m wondering if there may be a need for custom output formatters? Should it be possible to choose between the “% faster than baseline” or “% slower than baseline” formatter by using a command line flag?

@schneems
Copy link
Contributor

We could use x.compare!(:faster) and have it default to x.compare!(:slower) for a flag? Ultimately @evanphx is the gatekeeper here, I'm just throwing ideas at the wall.

When two pieces of code run the same number of iterations in the same
amount of time, that shouldn't be reported as one being "1.00x slower"
than the other. By comparing all slower results against the fastest
result, and not the other way around, we get some more realistic
reporting.
@evanphx
Copy link
Owner

evanphx commented Aug 12, 2015

Take a look at 7961c61 and give me your feedback please!

@evanphx evanphx closed this Apr 1, 2016
@bquorning
Copy link
Contributor Author

Why was this closed?

@evanphx
Copy link
Owner

evanphx commented Apr 3, 2016

No one responded to my feedback request 8 months ago.

@evanphx
Copy link
Owner

evanphx commented Apr 3, 2016

Or, rather, you did but the change seemed fine.

@bquorning bquorning deleted the more-understandable-ips-comparison branch April 6, 2018 09:31
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.

None yet

4 participants