evanphx / benchmark-ips

Provides iteration per second benchmarking for Ruby
MIT License
1.72k stars 97 forks source link

Avoid overflow during warmup when the cycles would become >= 2**31 #106

Closed eregon closed 4 years ago

eregon commented 4 years ago

Comparison: call: 4257382144.9 i/s send: 2128282445.4 i/s - 2.00x (± 0.00) slower method_missing: 2105291438.1 i/s - 2.02x (± 0.00) slower

* After:

Warming up -------------------------------------- call 428.105M i/100ms send 428.196M i/100ms method_missing 428.042M i/100ms Calculating ------------------------------------- call 4.298B (± 0.2%) i/s - 21.833B in 5.080217s send 4.295B (± 0.3%) i/s - 21.838B in 5.085086s method_missing 4.299B (± 0.2%) i/s - 21.830B in 5.078548s

Comparison: method_missing: 4298509323.4 i/s call: 4297742474.3 i/s - same-ish: difference falls within error send: 4294549352.9 i/s - same-ish: difference falls within error



@nateberkopec Could you review? :smiley: 
nateberkopec commented 4 years ago

Should we log something in this case? Or just blow up entirely and refuse to display a result?

Essentially we don't really know how fast this code is, because it iterates too quickly.

eregon commented 4 years ago

Essentially we don't really know how fast this code is, because it iterates too quickly.

Actually we do, the benchmark block is at least 4 Bips or faster, which is a result on its own.

Should we log something in this case? Or just blow up entirely and refuse to display a result?

I believe we should just let it be, here it is interesting to see they all optimize to very little code (a loop with a few checks inside) that appears to run at the same speed.

Also the "might have optimized away" is something that can happen without that overflow.

The PR doesn't change behavior, it just avoids a needless overflow which would affect the result and cause some ordering bias for the first benchmark block, which would confusingly show a difference when there isn't (the compiler graphs are exactly the same at least for call and send).

evanphx commented 4 years ago

This is a fine patch, we should push it out.

eregon commented 4 years ago

@evanphx Thanks for the review!

nateberkopec commented 4 years ago

Releasing now as 2.8.3

eregon commented 4 years ago

Thanks for the quick release!