Counter(): add 'one thousand' param. by LebedevRI · Pull Request #657 · google/benchmark

LebedevRI · 2018-08-19T09:41:52Z

Needed for #654

Custom user counters are quite custom. It is not guaranteed
that the user always expects for these to have 1k == 1000.
If the counter represents bytes/memory/etc, 1k should be 1024.

Some bikeshedding points:

Is this sufficient, or do we really want to go full on
into custom types with names?
I think just the '1000' is sufficient for now.
Should there be a helper benchmark::Counter::Counter{1000,1024}()
static 'constructor' functions, since these two, by far,
will be the most used?
In the future, we should be somehow encoding this info into JSON.

AppVeyorBot · 2018-08-19T09:52:21Z

❌ Build benchmark 1377 failed (commit 9dd521fc67 by @LebedevRI)

coveralls · 2018-08-19T09:56:03Z

Coverage increased (+0.7%) to 87.635% when pulling 2706e31 on LebedevRI:counter-thousand into d9cab61 on google:master.

AppVeyorBot · 2018-08-19T10:11:29Z

❌ Build benchmark 1378 failed (commit cb960fbba8 by @LebedevRI)

AppVeyorBot · 2018-08-19T11:07:17Z

❌ Build benchmark 1379 failed (commit 0d4a129ac2 by @LebedevRI)

AppVeyorBot · 2018-08-19T12:03:12Z

❌ Build benchmark 1380 failed (commit 28d2e3e708 by @LebedevRI)

AppVeyorBot · 2018-08-19T13:10:20Z

✅ Build benchmark 1381 completed (commit d7a05b3067 by @LebedevRI)

AppVeyorBot · 2018-08-19T14:15:24Z

✅ Build benchmark 1382 completed (commit a40d39a42e by @LebedevRI)

EricWF · 2018-08-19T15:00:05Z

I haven't looked at this, but my preference would be to encode this info in the type system in some way. Sort of like std::ratio.

So maybe make the parameter something similar instead of just taking an integer?

LebedevRI · 2018-08-19T15:07:19Z

I'm not sure how that would look. Just take std::pair<int /*num*/, int /*den*/> ?

AppVeyorBot · 2018-08-19T16:47:57Z

✅ Build benchmark 1383 completed (commit f79a0f2a51 by @LebedevRI)

LebedevRI · 2018-08-27T06:06:28Z

Ping.

dmah42 · 2018-08-28T14:16:35Z

src/console_reporter.cc

                                          c.first.length());
-    auto const& s = HumanReadableNumber(c.second.value, 1000);
+    const double one_k =
+        double(c.second.thousand.first) / double(c.second.thousand.second);


why is this specified as a pair instead of just a single number (1000 vs 1024)? or even an enum!

I would personally think just one value is enough (which is what i had in the first commit, d4a5af5),
but #657 (comment) may or may not suggest that something more advanced is needed.
@EricWF ?

Ah, I think that is suggesting either having an enum with values 1000 and 1024, or to extend Counter to have MegaCounter and MebiCounter variants, which implicitly manage the one_k definition without it being specified.

Right now i don't have/is not aware of any real use for the one_k not being 1000 or 1024, so enum could work.
It is just that storing enum will take just as much space as storing an int, or a pair of shorts:
https://godbolt.org/z/9Q1gVS
So it isn't obvious to me why we want to avoid doing/allowing the more general thing at the same cost.

It's just.. weird. Like, why would you define one_k as 2000/2 or 500/PI?

LebedevRI · 2018-08-29T13:08:24Z

include/benchmark/benchmark.h


  double value;
  Flags flags;
+  std::pair<int /*Num*/, int /*Denom*/> thousand;


Hmm, if it to be an enum, should i just add value k1024 to the Flags enum?

I think it's an orthogonal feature so i'd add a OneK enum with two values.

Ok, sounds good.

dmah42 · 2018-08-29T14:02:12Z

src/console_reporter.cc

                                          c.first.length());
-    const double one_k =
-        double(c.second.thousand.first) / double(c.second.thousand.second);
+    const double one_k = [](benchmark::Counter::One_K k) -> double {


why not give the enums the values '1000' and '1024' and pass them through (with any necessary cast) to the HumanReadableNumber call?

There is no real reason not to do it.
I didn't do that because 0 and 1 (which is most likely the current values of these values)
only need one bit, while 1024 needs 11 bits. So if we wanted to pack the size of Counter,
it would likely be simpler with smaller enum values.
I don't think we care about that here, so let's avoid this premature optimization :)

AppVeyorBot · 2018-08-29T14:28:14Z

❌ Build benchmark 1396 failed (commit 262e146e02 by @LebedevRI)

Needed for google#654 Custom user counters are quite custom. It is not guaranteed that the user *always* expects for these to have 1k == 1000. If the counter represents bytes/memory/etc, 1k should be 1024. Some bikeshedding points: 1. Is this sufficient, or do we really want to go full on into custom types with names? I think just the '1000' is sufficient for now. 2. Should there be a helper benchmark::Counter::Counter{1000,1024}() static 'constructor' functions, since these two, by far, will be the most used? 3. In the future, we should be somehow encoding this info into JSON.

Simpler is better. If someone comes up with a real reason to need something more advanced, it can be added later on.

AppVeyorBot · 2018-08-29T15:56:11Z

✅ Build benchmark 1397 completed (commit 506fe68a83 by @LebedevRI)

dmah42 · 2018-08-29T16:07:08Z

include/benchmark/benchmark.h

    kAvgIterationsRate = kIsRate | kAvgIterations
  };

+  enum One_K {


nit: https://google.github.io/styleguide/cppguide.html#Enumerator_Names

OneK is a better name.

LebedevRI · 2018-08-29T16:43:41Z

Thank you for the review!
Awaiting CI completion..

AppVeyorBot · 2018-08-29T18:10:40Z

✅ Build benchmark 1398 completed (commit 4a40148f12 by @LebedevRI)

* Counter(): add 'one thousand' param. Needed for google#654 Custom user counters are quite custom. It is not guaranteed that the user *always* expects for these to have 1k == 1000. If the counter represents bytes/memory/etc, 1k should be 1024. Some bikeshedding points: 1. Is this sufficient, or do we really want to go full on into custom types with names? I think just the '1000' is sufficient for now. 2. Should there be a helper benchmark::Counter::Counter{1000,1024}() static 'constructor' functions, since these two, by far, will be the most used? 3. In the future, we should be somehow encoding this info into JSON. * Counter(): use std::pair<> to represent 'one thousand' * Counter(): just use a new enum with two values 1000 vs 1024. Simpler is better. If someone comes up with a real reason to need something more advanced, it can be added later on. * Counter: just store the 1000 or 1024 in the One_K values directly * Counter: s/One_K/OneK/

googlebot added the cla: yes label Aug 19, 2018

LebedevRI force-pushed the counter-thousand branch from 3d325d6 to db8dfb4 Compare August 19, 2018 10:00

LebedevRI force-pushed the counter-thousand branch from db8dfb4 to 05bde77 Compare August 19, 2018 10:15

LebedevRI force-pushed the counter-thousand branch from 05bde77 to 44b035e Compare August 19, 2018 11:11

LebedevRI force-pushed the counter-thousand branch 2 times, most recently from 61194b4 to d4a5af5 Compare August 19, 2018 12:06

LebedevRI requested review from EricWF and dmah42 August 19, 2018 13:10

dmah42 reviewed Aug 28, 2018

View reviewed changes

LebedevRI commented Aug 29, 2018

View reviewed changes

dmah42 reviewed Aug 29, 2018

View reviewed changes

LebedevRI added 4 commits August 29, 2018 17:34

Counter(): use std::pair<> to represent 'one thousand'

93abc3f

Counter(): just use a new enum with two values 1000 vs 1024.

a92e4b6

Simpler is better. If someone comes up with a real reason to need something more advanced, it can be added later on.

Counter: just store the 1000 or 1024 in the One_K values directly

fba1dfb

LebedevRI force-pushed the counter-thousand branch from 3baebbc to fba1dfb Compare August 29, 2018 14:35

dmah42 reviewed Aug 29, 2018

View reviewed changes

dmah42 approved these changes Aug 29, 2018

View reviewed changes

Counter: s/One_K/OneK/

2706e31

LebedevRI merged commit caa2fcb into google:master Aug 29, 2018

LebedevRI deleted the counter-thousand branch August 29, 2018 18:11

Conversation

LebedevRI commented Aug 19, 2018

Uh oh!

AppVeyorBot commented Aug 19, 2018

Uh oh!

coveralls commented Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AppVeyorBot commented Aug 19, 2018

Uh oh!

AppVeyorBot commented Aug 19, 2018

Uh oh!

AppVeyorBot commented Aug 19, 2018

Uh oh!

AppVeyorBot commented Aug 19, 2018

Uh oh!

AppVeyorBot commented Aug 19, 2018

Uh oh!

EricWF commented Aug 19, 2018

Uh oh!

LebedevRI commented Aug 19, 2018

Uh oh!

AppVeyorBot commented Aug 19, 2018

Uh oh!

LebedevRI commented Aug 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Aug 29, 2018

Uh oh!

AppVeyorBot commented Aug 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LebedevRI commented Aug 29, 2018

Uh oh!

AppVeyorBot commented Aug 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coveralls commented Aug 19, 2018 •

edited

Loading