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

Formatting improvements. #20

Closed
wants to merge 2 commits into from
Closed

Formatting improvements. #20

wants to merge 2 commits into from

Conversation

headius
Copy link
Contributor

@headius headius commented Nov 10, 2014

  • Scale iter counts by 10^3s with suffix, for better readability.
  • Reduce printed precision to 3 decimal places throughout.
  • Fix stdddev percentage width to 4 characters, for alignment.

Here's the outputs before and after. I was always having trouble reading ips counts when they get into the hundreds of thousands or higher, and the stddev alignment thing was just bugging me. The ips scaling change also helps keep alignment consistent.

The precision changes warrant some justification. For the time elapsed, I figured that anything lower than ms is not useful for an "iterations per second" benchmark, and the only times shown are all around 5s range. For i/s, anything scaled 1/1000 is going to be in the neighborhood of benchmark noise. If something can do 50M iterations in 5 seconds, that's 10M per second, and 1000 iterations (the 1/10000 lost precision) takes 0.1ms. The error rate for that benchmark is +/- 1000 iterations, or 0.0002%. Acceptable to have good, readable results.

BEFORE:

Calculating -------------------------------------
uncontended MD5 'foo'
                         29296 i/100ms
uncontended MD5 'foo' * 1000
                          9558 i/100ms
4x contended MD5 'foo'
                           599 i/100ms
4x contended MD5 'foo' * 1000
                           519 i/100ms
10x contended MD5 'foo'
                           267 i/100ms
10x contended MD5 'foo' * 1000
                           229 i/100ms
100x contended MD5 'foo'
                            21 i/100ms
100x contended MD5 'foo' * 1000
                            19 i/100ms
-------------------------------------------------
uncontended MD5 'foo'
                       543446.5 (±5.2%) i/s -    2724528 in   5.028890s
uncontended MD5 'foo' * 1000
                       134059.2 (±2.9%) i/s -     678618 in   5.066376s
4x contended MD5 'foo'
                       123911.7 (±10.6%) i/s -     609782 in   4.994339s
4x contended MD5 'foo' * 1000
                        31893.7 (±6.9%) i/s -     158814 in   5.006711s
10x contended MD5 'foo'
                        47335.5 (±9.4%) i/s -     233625 in   4.994032s
10x contended MD5 'foo' * 1000
                        12726.5 (±5.5%) i/s -      63433 in   5.001239s
100x contended MD5 'foo'
                         2463.1 (±8.1%) i/s -      12222 in   4.999318s
100x contended MD5 'foo' * 1000
                          976.0 (±5.2%) i/s -       4883 in   5.018057s

AFTER:

Calculating -------------------------------------
uncontended MD5 'foo'
                        29.733k i/100ms
uncontended MD5 'foo' * 1000
                         9.572k i/100ms
4x contended MD5 'foo'
                       618.000  i/100ms
4x contended MD5 'foo' * 1000
                       531.000  i/100ms
10x contended MD5 'foo'
                       273.000  i/100ms
10x contended MD5 'foo' * 1000
                       234.000  i/100ms
100x contended MD5 'foo'
                        22.000  i/100ms
100x contended MD5 'foo' * 1000
                        19.000  i/100ms
-------------------------------------------------
uncontended MD5 'foo'
                        553.452k (± 3.5%) i/s -      2.765M in      5.003s
uncontended MD5 'foo' * 1000
                        134.654k (± 2.9%) i/s -    679.612k in      5.051s
4x contended MD5 'foo'
                        124.699k (±10.1%) i/s -    614.292k in      4.994s
4x contended MD5 'foo' * 1000
                         32.174k (± 6.9%) i/s -    160.362k in      5.012s
10x contended MD5 'foo'
                         47.166k (± 9.2%) i/s -    233.142k in      4.999s
10x contended MD5 'foo' * 1000
                         12.643k (± 5.6%) i/s -     63.180k in      5.015s
100x contended MD5 'foo'
                          2.513k (± 7.2%) i/s -     12.496k in      5.002s
100x contended MD5 'foo' * 1000
                        978.344  (± 5.1%) i/s -      4.883k in      5.006s

* Scale iter counts by 10^3s with suffix, for better readability.
* Reduce printed precision to 3 decimal places throughout.
* Fix stdddev percentage width to 4 characters, for alignment.
@evanphx
Copy link
Owner

evanphx commented Nov 10, 2014

My big worry is the magnitude switching between lines. It's easy to miss the magnitude (even though you did reserve it a whole column) when looking at the numbers. What I'd prefer is to look at all the numbers and decide "ok, most of these are above 1000, we should display them all in kilo" so that we don't have switching between lines.

@enebo
Copy link

enebo commented Nov 10, 2014

@evanphx Any possibility of getting rid of '1000000000 in' from this line. Does that convey more info than rate with total time (e.g. it is derivable and I never care about this number)?

Apologies for jumping into this bug but it is about formatting :)

@evanphx
Copy link
Owner

evanphx commented Nov 10, 2014

@enebo Which line?

@enebo
Copy link

enebo commented Nov 10, 2014

978.344  (± 5.1%) i/s -      4.883k in      5.006s
978.344  (± 5.1%) i/s  over  5.006s

@headius
Copy link
Contributor Author

headius commented Nov 10, 2014

Unfortunately we can't pick a single scale for all numbers because they display their results immediately after executing. We'd have to wait and do a single report, and I think that hurts usability.

The scale changing concerned me at first, but I've had no trouble at all reading these suffixes. We're used to doing that with so many other aspects of computing...free space, time since last post (5s ago, 5m ago)...I don't see it as a problem.

And at any rate, it's MUCH easier to read 264.368M than 264368526. The latter causes me many more headaches than a changing suffix would.

@evanphx
Copy link
Owner

evanphx commented Nov 10, 2014

Ah! The in X.Ys is just for full disclosure. You're right, it doesn't convey much info other than to provide a quick sanity check that the test actually run the proper amount of time. In fact, benchmark/ips really needs to check those times and blow up if they're too far from the desired runtime.

@headius
Copy link
Contributor Author

headius commented Nov 10, 2014

Another case for suffix being more readable than exact value: it's harder to reasonably align columns if some results are 1 char wide and some results are twelve chars wide. This makes all results consistent width, followed by a scale suffix. Easier! :-)

@enebo
Copy link

enebo commented Nov 10, 2014

@evanphx yeah ultimately I don't care about 5.0002s either but I would care if the delta from a desired 5s run was too large.

@evanphx
Copy link
Owner

evanphx commented Nov 10, 2014

@headius Yeah, I can absolutely see the suffix being easier on those large numbers. I'm probably being a little overprotective. How about adding an option called ":human" that is on by default and when set, uses the prefixes. That at least gives people an easy out.

@headius
Copy link
Contributor Author

headius commented Nov 10, 2014

I thought about adding an option and I agree it would be good to have a "raw" form. I'll do as you suggest.

@evanphx
Copy link
Owner

evanphx commented Nov 10, 2014

@enebo I agree that the in X.Y ends up being noise. I'll change it so that it's suppressed unless the value is 5% outside the desired runtime.

@headius
Copy link
Contributor Author

headius commented Nov 10, 2014

@evanphx I don't see any options for IPS other than those passed into the Benchmark.ips method. I have a patch to add them there (https://gist.github.com/headius/61c09c2c5b8574b30df4). Is this what you meant? My gut is telling me this should be an option you can toggle without changing the benchmark, though.

@evanphx
Copy link
Owner

evanphx commented Nov 10, 2014

That patch is fine. You could add a Hash at Benchmark::IPS.options that contains the defaults so people can do Benchmark::IPS.options[:format] = :raw.

@evanphx
Copy link
Owner

evanphx commented Nov 10, 2014

@enebo 4736eb6 for you!

@headius
Copy link
Contributor Author

headius commented Nov 10, 2014

I moved the option out of the ips() call and solely into Benchmark::IPS.options. It just didn't feel right to have the benchmark hardcode its format.

@@ -68,8 +73,14 @@ def stddev_percentage
# percentage of standard deviation, iterations in runtime.
# @return [String] Left justified body.
def body
left = "%10.1f (±%.1f%%) i/s" % [ips, stddev_percentage]
left.ljust(20) + (" - %10d in %10.6fs" % [@iterations, runtime])
case Benchmark::IPS.options[:format]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to use @format here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified it to use @Format in that first patch, but then decided that the jobs shouldn't really aggregate a format; they should get it from an external config source so it can be changed without changing the jobs. They all go after it via Benchmark::IPS.options now

@evanphx
Copy link
Owner

evanphx commented Nov 10, 2014

@headius Ok, you need to remove the other @format stuff in there too then.

@headius
Copy link
Contributor Author

headius commented Nov 10, 2014

Oops, silly me. Removed the rogue bits and force pushed.

@@ -20,6 +20,9 @@ module IPS
# iteration per second with standard deviation in given time.
# @param time [Integer] Specify how long should benchmark your code in seconds.
# @param warmup [Integer] Specify how long should Warmup time run in seconds.
# @param format [Symbol] Specify formatting of iteration counts. :human will
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to remove this comment too, there is no format arg.

@evanphx
Copy link
Owner

evanphx commented Nov 10, 2014

@headius one more fix and we'll be there!

@headius
Copy link
Contributor Author

headius commented Nov 10, 2014

Yeesh...guess I have too many distractions today. Re-pushed.

@evanphx
Copy link
Owner

evanphx commented Nov 10, 2014

I rebased it again against my latest commit and committed it. Thanks!

@evanphx evanphx closed this Nov 10, 2014
@krainboltgreene
Copy link

The dot notation for the numbers is really throwing me off, but I also can't seem to convert to raw. I've tried passing it to the block object, as a argument to the #ips method, and more. How am I supposed to not use this new format for numbers?

@krainboltgreene
Copy link

Not only can I not find the way to set it, I'm fairly sure there is no way to set this value. There are no tests for this either.

@evanphx
Copy link
Owner

evanphx commented Apr 8, 2015

@krainboltgreene You can set it back to raw by adding Benchmark::IPS.options[:format] = :raw

@evanphx evanphx reopened this Apr 8, 2015
@evanphx evanphx closed this Apr 8, 2015
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