Add optional ms time unit for console reporter #192

NewProggie · 2016-03-24T21:21:53Z

Some benchmarks may run a few milliseconds which makes it kind of hard to visually compare, since the currently only available nanoseconds numbers can get very large in this case. Therefore this commit adds an optional command line flag --benchmark_time_unit which lets the user choose between ns and ms time units for displaying the mean execution time.

Some benchmarks may run a few milliseconds which makes it kind of hard to visually compare, since the currently only available nanoseconds numbers can get very large in this case. Therefore this commit adds an optional command line flag --benchmark_time_unit which lets the user choose between ns and ms time units for displaying the mean execution time.

googlebot · 2016-03-24T21:21:56Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

NewProggie · 2016-03-24T21:24:00Z

I signed it!

googlebot · 2016-03-24T21:24:01Z

CLAs look good, thanks!

coveralls · 2016-03-24T21:26:24Z

Coverage increased (+0.2%) to 68.525% when pulling cded70a on NewProggie:feature/add-ms-time-report into b2e7340 on google:master.

coveralls · 2016-03-24T21:39:27Z

Coverage increased (+0.2%) to 68.525% when pulling 3a02c46 on NewProggie:feature/add-ms-time-report into b2e7340 on google:master.

dmah42 · 2016-03-24T22:25:56Z

src/console_reporter.cc

  }

-  double const multiplier = 1e9; // nano second multiplier
+  double const multiplier = time_unit_ == "ns" ? 1e9 : 1e3; // nano second or


we take pains to make other values human readable. could we do the same here?

ie, instead of a flag, can we add the unit suffix to the output and automatically detect if the value should be ns, us, ms, etc?

Ah, interesting. Say, for example the measured values are above a certain number the unit should switch to the next greater one (from ns -> us -> ms -> s)? The unit in the header row would be removed then. I would propose a time unit range from nanoseconds up to seconds (we can safely ditch the planck time here, for sure :-) )

right. that's essentially what we do with the values that go into the benchmark name.

it might make at-a-glance comparison a little more awkward if a single benchmark jumps between units. i'm not sure what's best.

i do know that flags are clumsy :)

Yes, indeed.
In order to stay backwards compatible, I'd suggest to keep the default behaviour the way it is (all values in ns). If there shouldn't be another commandline flag, we could just add more rows for ms and seconds besides the already existing ns row. In this case at-a-glance comparisons would still be possible for one unit. Otherwise I could change the commandline flag (as well as the implementation) to move units directly to the output.

i don't think anyone is parsing the console output so just changing it should be fine.

we have csv and json that is more readily parseable, and shouldn't be affected by this change.

pleroy · 2016-03-25T08:49:28Z

Would it make sense to specify the unit in the benchmark registration itself? Something like:

BENCHMARK(BM_NewhallApproximation)->Arg(4)->Arg(8)->Arg(16)->Unit(kMillisecond);

I'd say that when you write a benchmark you have a reasonable idea of the time it will take. The notion of having the reporting magically decide what order of magnitude works best worries me: in the above example it would be very inconvenient if the run for Arg(4) used microseconds but the run for Arg(16) used milliseconds.

dmah42 · 2016-03-25T18:22:41Z

That's an interesting compromise.

I have a strong preference for not requiring configuration, so i'd refactor things to run all the runs for a benchmark arg set and then decide which unit to present for the set.

However, if @NewProggie wants to add this as an option and see how it plays out, i'd love to see it.

coveralls · 2016-03-26T00:09:45Z

Coverage increased (+0.9%) to 69.231% when pulling 7c69b36 on NewProggie:feature/add-ms-time-report into b2e7340 on google:master.

NewProggie · 2016-03-26T00:17:22Z

Gathering all times for all the runs before setting a time unit and reporting it would be more work I guess. Furthermore one would not see any measured times before the last run for a benchmark suite has finished (which might take a longer time). This is unfortunate.

dmah42 · 2016-03-28T15:55:00Z

src/console_reporter.cc

              result.report_label.c_str());
 }

+TimeUnitMultiplier ConsoleReporter::getTimeUnitAndMultiplier(TimeUnit unit) {


If i've asked a benchmark to report in ms, it should be reported in ms no matter which reporter I use.

as such, this can be moved to BenchmarkReporter and called from each of csv, json, and console reporters.

This comment is unclear to me, since csv and json reporter currently don't display the time unit. Nevertheless I moved the function to BenchmarkReporter as you suggested.

right, but they should now scale appropriately given the user has requested a particular time scale.

they should maybe have a field for the time unit added too...

NewProggie · 2016-03-28T19:36:00Z

By the way: I've noticed that the code formatting is inconsistent in several areas, but didn't want to pollute this pull request with reformatting. I could provide an appropriate clang-format configuration (basically: use Style: Google) and open another pull request for this?

coveralls · 2016-03-28T19:37:02Z

Coverage increased (+0.9%) to 69.231% when pulling 0b4111c on NewProggie:feature/add-ms-time-report into b2e7340 on google:master.

dmah42 · 2016-03-28T21:14:06Z

test/CMakeLists.txt

 # Enable the tests

+# Allow the source files to find headers in src/
+include_directories(${PROJECT_SOURCE_DIR}/src)


you shouldn't need this. the only extra thing here is the time unit which is already in include/benchmark.

Actually, yes. I was using SleepForMilliseconds in my test in order to get appropriate timings for each time unit.

ah right. it's a shame to break the abstraction here and have tests that seem to rely on internals. could you add a comment to the CMakeLists.txt explaining why it's necessary?

Yes, this is unfortunate. Maybe we should use a different approach here, such as a bad implemented fibonacci function which generates the work load and ditch the SleepForMilliseconds inside the test alltogether?

whatever you want to do is fine. sleeping is ok, as long as we're clear in the cmake config why we're relying on src. i'd hate for users to read that as an example and think they need internals for some reason.

see #199

the benchmark test there uses std::this_thread::sleep_for which doesn't require pulling from src.

….txt

coveralls · 2016-04-29T20:02:09Z

Coverage increased (+0.7%) to 69.005% when pulling f352c30 on NewProggie:feature/add-ms-time-report into 0500ec0 on google:master.

coveralls · 2016-05-02T18:01:35Z

Coverage increased (+0.1%) to 68.464% when pulling fb73389 on NewProggie:feature/add-ms-time-report into 0500ec0 on google:master.

dmah42 · 2016-05-02T18:17:54Z

Thank you!

NewProggie · 2016-05-02T18:20:47Z

Thanks as well. I've learned some new things along the way ;-)

googlebot added the cla: no label Mar 24, 2016

googlebot added cla: yes and removed cla: no labels Mar 24, 2016

Add myself to the contributors list

3a02c46

dmah42 reviewed Mar 24, 2016
View reviewed changes

Add an additional parameter for time units

7c69b36

dmah42 reviewed Mar 28, 2016
View reviewed changes

Refactor GetTimeUnitAndMultiplier and add example

0b4111c

dmah42 reviewed Mar 28, 2016
View reviewed changes

NewProggie added 4 commits March 29, 2016 20:35

Add GetTimeAndMultiplier to json and csv reporter as well

e6d62fd

Add explanatory comment why src headers are needed in test/CMakeLists…

1043f45

….txt

Fix missing <tuple> header in csv/json_reporter

1203b3c

Merge branch 'master' into feature/add-ms-time-report

f352c30

Remove sleep.h dependency for tests

fb73389

dmah42 merged commit fb73389 into google:master May 2, 2016

NewProggie deleted the feature/add-ms-time-report branch May 2, 2016 18:20

Add optional ms time unit for console reporter #192

Add optional ms time unit for console reporter #192

Uh oh!

Conversation

NewProggie commented Mar 24, 2016

Uh oh!

googlebot commented Mar 24, 2016

Uh oh!

NewProggie commented Mar 24, 2016

Uh oh!

googlebot commented Mar 24, 2016

Uh oh!

coveralls commented Mar 24, 2016

Uh oh!

coveralls commented Mar 24, 2016

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!

pleroy commented Mar 25, 2016

Uh oh!

dmah42 commented Mar 25, 2016

Uh oh!

coveralls commented Mar 26, 2016

Uh oh!

NewProggie commented Mar 26, 2016

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!

NewProggie commented Mar 28, 2016

Uh oh!

coveralls commented Mar 28, 2016

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!

coveralls commented Apr 29, 2016

Uh oh!

coveralls commented May 2, 2016

Uh oh!

dmah42 commented May 2, 2016

Uh oh!

NewProggie commented May 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants