Add command line option to print user counters in tabular format #350

biojppm · 2017-03-02T02:58:50Z

This PR adds the option --benchmark_counters_tabular={true|false}, which makes the ConsoleReporter print the user counters in tabular format. The option defaults to false. This follows discussions in PR #262 (notably this and this).

To be clear:

by default, counters are printed at the end, the same way as bytes_processed and items_processed, as agreed in Add user-defined counters. #262 . This is best for cases in which there are few counters, or where there are only a couple of lines per benchmark.
when --benchmark_counters_tabular=true is given, then the counters are printed as a column of the table. This is best for cases in which there are a lot of counters, or a lot of lines per benchmark.

Here's an example comparison between the two versions (Note that I removed some whitespace from both examples).

First the default --benchmark_counters_tabular=false:

------------------------------------------------------------------------------
Benchmark                        Time           CPU Iterations UserCounters...
------------------------------------------------------------------------------
BM_UserCounter/threads:8      2248 ns      10277 ns      68808 Bar=16 Bat=40 Baz=24 Foo=8
BM_UserCounter/threads:1      9797 ns       9788 ns      71523 Bar=2 Bat=5 Baz=3 Foo=1024m
BM_UserCounter/threads:2      4924 ns       9842 ns      71036 Bar=4 Bat=10 Baz=6 Foo=2
BM_UserCounter/threads:4      2589 ns      10284 ns      68012 Bar=8 Bat=20 Baz=12 Foo=4
BM_UserCounter/threads:8      2212 ns      10287 ns      68040 Bar=16 Bat=40 Baz=24 Foo=8
BM_UserCounter/threads:16     1782 ns      10278 ns      68144 Bar=32 Bat=80 Baz=48 Foo=16
BM_UserCounter/threads:32     1291 ns      10296 ns      68256 Bar=64 Bat=160 Baz=96 Foo=32
BM_UserCounter/threads:4      2615 ns      10307 ns      68040 Bar=8 Bat=20 Baz=12 Foo=4
BM_Factorial                    26 ns         26 ns   26608979 40320
BM_Factorial/real_time          26 ns         26 ns   26587936 40320
BM_CalculatePiRange/1           16 ns         16 ns   45704255 0
BM_CalculatePiRange/8           73 ns         73 ns    9520927 3.28374
BM_CalculatePiRange/64         609 ns        609 ns    1140647 3.15746
BM_CalculatePiRange/512       4900 ns       4901 ns     142696 3.14355

... compared to the tabular version --benchmark_counters_tabular=true. Note the additional printing of the header. This will happen any time the counter set changes.

---------------------------------------------------------------------------------------
Benchmark                        Time           CPU Iterations    Bar   Bat   Baz   Foo
---------------------------------------------------------------------------------------
BM_UserCounter/threads:8      2198 ns       9953 ns      70688     16    40    24     8
BM_UserCounter/threads:1      9504 ns       9504 ns      73787      2     5     3     1
BM_UserCounter/threads:2      4775 ns       9550 ns      72606      4    10     6     2
BM_UserCounter/threads:4      2508 ns       9951 ns      70332      8    20    12     4
BM_UserCounter/threads:8      2055 ns       9933 ns      70344     16    40    24     8
BM_UserCounter/threads:16     1610 ns       9946 ns      70720     32    80    48    16
BM_UserCounter/threads:32     1192 ns       9948 ns      70496     64   160    96    32
BM_UserCounter/threads:4      2506 ns       9949 ns      70332      8    20    12     4
--------------------------------------------------------------
Benchmark                        Time           CPU Iterations
--------------------------------------------------------------
BM_Factorial                    26 ns         26 ns   26392245 40320
BM_Factorial/real_time          26 ns         26 ns   26494107 40320
BM_CalculatePiRange/1           15 ns         15 ns   45571597 0
BM_CalculatePiRange/8           74 ns         74 ns    9450212 3.28374
BM_CalculatePiRange/64         595 ns        595 ns    1173901 3.15746
BM_CalculatePiRange/512       4752 ns       4752 ns     147380 3.14355
BM_CalculatePiRange/4k       37970 ns      37972 ns      18453 3.14184
BM_CalculatePiRange/32k     303733 ns     303744 ns       2305 3.14162
BM_CalculatePiRange/256k   2434095 ns    2434186 ns        288 3.1416
BM_CalculatePiRange/1024k  9721140 ns    9721413 ns         71 3.14159
BM_CalculatePi/threads:8      2255 ns       9943 ns      70936

AppVeyorBot · 2017-03-02T02:59:35Z

❌ Build benchmark 540 failed (commit e0e3889b33 by @biojppm)

coveralls · 2017-03-02T03:06:36Z

Changes Unknown when pulling 0de985a on biojppm:compact into ** on google:master**.

dmah42 · 2017-03-02T05:13:52Z

src/benchmark.cc

+      output_opts &= ~ConsoleReporter::OO_Tabular;
+    }
+    default_console_reporter = internal::CreateReporter(
+          FLAGS_benchmark_format, (ConsoleReporter::OutputOptions)output_opts);


use c++-style casts please.

dmah42 · 2017-03-02T05:16:05Z

src/console_reporter.cc

+  prev_counters_.clear();

  PrintBasicContext(&GetErrorStream(), context);



don't forget to update the WINDOWS build too. color_output_ is referenced in there.

AppVeyorBot · 2017-03-02T12:12:26Z

❌ Build benchmark 541 failed (commit 4ec49b8a30 by @biojppm)

coveralls · 2017-03-02T12:18:35Z

Changes Unknown when pulling c846eed on biojppm:compact into ** on google:master**.

coveralls · 2017-03-02T12:25:46Z

Changes Unknown when pulling d84d911 on biojppm:compact into ** on google:master**.

AppVeyorBot · 2017-03-02T12:42:30Z

❌ Build benchmark 542 failed (commit 10bc6154fe by @biojppm)

dmah42 · 2017-03-02T17:30:30Z

Looks like one of the tests is failing for MinGW relating to console output. Could you take a look?

biojppm · 2017-04-01T21:33:36Z

@dominichamon Sorry for the late reply. I did see that MinGW fail when I first pushed; I thought it was a spurious fail which I've sometimes seen with the output test.

Looking more closely, it is indeed something in the output test, and AFAICT it is unrelated to any counter logic; here's the message:

C:\projects\benchmark\test\output_test_helper.cc:93: CheckCase: Check `remaining_output.eof() == false' failed. End of output reached before match for regex "^BM_Repeat/repeats:3 %console_report$" was found
    actual regex string "^BM_Repeat/repeats:3 [ ]*[0-9]{1,5} ns [ ]*[0-9]{1,5} ns [ ]*[0-9]+$"
    started matching near: BM_Repeat/repeats:3                            4 ns         -0 ns    1000000
This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information```

coveralls · 2017-04-01T21:41:48Z

Changes Unknown when pulling 707dd89 on biojppm:compact into ** on google:master**.

AppVeyorBot · 2017-04-01T21:57:32Z

✅ Build benchmark 570 completed (commit 3ce9f21d60 by @biojppm)

biojppm · 2017-04-01T21:59:34Z

@dominichamon Indeed, the (same) PR is now passing Appveyor's MinGW test.

dmah42 · 2017-05-02T18:00:51Z

src/console_reporter.cc

+  if(!run.counters.empty()) {
+    if(output_options_ & OO_Tabular) {
+      for(auto const& c : run.counters) {
+        str += FormatString(" %10s", c.first);


c.first.c_str() i think

AppVeyorBot · 2017-05-02T18:04:20Z

❌ Build benchmark 616 failed (commit d5091285f8 by @biojppm)

coveralls · 2017-05-02T22:09:54Z

Coverage increased (+1.0%) to 86.486% when pulling 020bac9 on biojppm:compact into da8cd74 on google:master.

dmah42 · 2017-05-02T22:14:44Z

src/benchmark_api_internal.h

+
 namespace {

 bool IsZero(double n) {


unused function, apparently? in travis (clang release)

I've fixed by pragma-ignoring it, is it OK?

dmah42 · 2017-05-02T22:15:09Z

src/csv_reporter.cc

+    if(it == run.counters.end()) {
+      Out << ",";
+    }
+    else {


this dropped 'else' isn't google style. can you run this change through clang-format with Google style selected?

coveralls · 2017-05-02T22:34:40Z

Coverage increased (+1.0%) to 86.486% when pulling a310886 on biojppm:compact into da8cd74 on google:master.

AppVeyorBot · 2017-05-02T22:37:40Z

❌ Build benchmark 617 failed (commit b0458113f2 by @biojppm)

coveralls · 2017-05-02T22:46:21Z

Coverage increased (+1.0%) to 86.486% when pulling 1735413 on biojppm:compact into da8cd74 on google:master.

biojppm · 2017-05-02T22:46:50Z

@dominichamon The previous push happened by accident. This push (or more precisely this push sequence) now completes the PR with unit tests. There may be some details which are so-so (the pragma ignore for -Wunused-function comes to mind, or the // clang format off), but other than that I think that's it.

AppVeyorBot · 2017-05-02T22:53:49Z

❌ Build benchmark 618 failed (commit 09866a07f8 by @biojppm)

This thing with the pragma ignore was getting out of hand: now MinGW (and probably GCC) was erroring too. So I chose to move the definition of IsZero() out of the anonymous namespace into benchmark.cc.

biojppm · 2017-05-02T23:08:17Z

Hmmm. This thing with the pragma ignore is getting out of hand: now MinGW is complaining. I took the liberty of moving the definition of IsZero() out of the anonymous namespace into benchmark.cc.

coveralls · 2017-05-02T23:15:09Z

Coverage increased (+1.0%) to 86.486% when pulling ec6f035 on biojppm:compact into da8cd74 on google:master.

AppVeyorBot · 2017-05-02T23:30:21Z

✅ Build benchmark 619 completed (commit 02184fedf1 by @biojppm)

AppVeyorBot · 2017-05-03T00:06:47Z

✅ Build benchmark 620 completed (commit 03d2027393 by @biojppm)

Add command line option --benchmark_counters_tabular

0de985a

googlebot added the cla: yes label Mar 2, 2017

biojppm mentioned this pull request Mar 2, 2017

Add command line option to print user counters in tabular format #347

Closed

dmah42 requested changes Mar 2, 2017

View reviewed changes

dmah42 reviewed Mar 2, 2017

View reviewed changes

biojppm added 2 commits March 2, 2017 12:10

Tabular counters: fix dangling reference to color_output_.

cbb2eb8

Fix use of C-style cast.

c846eed

Tabular counters: fix missing cast on MSVC.

d84d911

dmah42 approved these changes Mar 2, 2017

View reviewed changes

User counters: document tabular output in README.md

707dd89

biojppm mentioned this pull request Apr 1, 2017

Refactor csv and console reporting to robustly handle counters. #349

Closed

This was referenced May 2, 2017

Add unit tests for user counters. #381

Merged

reporter output test: MinGW intermittently returns negative times for BM_Repeat, test fails #382

Closed

Merge remote-tracking branch 'upstream/master' into compact

6151517

dmah42 reviewed May 2, 2017

View reviewed changes

biojppm added 3 commits May 2, 2017 20:31

Fix: --benchmark_counters_tabular was not being passed to tests.

17a012d

Add first unit test for benchmark_tabular_counters.

c69b385

Add test for tabular output of rate counters.

cf20dc9

biojppm added 4 commits May 2, 2017 20:48

Console reporter: add /s prefix to counter rates.

3db6254

Allow different counter sets in CSV reporting.

ea019f3

Add unit test for counter sets.

2506044

Extend tabular counter tests to different counter sets.

020bac9

dmah42 reviewed May 2, 2017

View reviewed changes

biojppm added 2 commits May 2, 2017 23:25

Fix (that is, ignore) clang compile error.

a310886

Fix dropped-style elses.

160770f

Fix pragma clang ignore with gcc.

1735413

Trying again to fix error caused by -Wunused-function.

ec6f035

This thing with the pragma ignore was getting out of hand: now MinGW (and probably GCC) was erroring too. So I chose to move the definition of IsZero() out of the anonymous namespace into benchmark.cc.

dmah42 merged commit ec6f035 into google:master May 3, 2017

		prev_counters_.clear();

		PrintBasicContext(&GetErrorStream(), context);

Add command line option to print user counters in tabular format #350

Add command line option to print user counters in tabular format #350

Uh oh!

Conversation

biojppm commented Mar 2, 2017

Uh oh!

AppVeyorBot commented Mar 2, 2017

Uh oh!

coveralls commented Mar 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Mar 2, 2017

Uh oh!

coveralls commented Mar 2, 2017

Uh oh!

coveralls commented Mar 2, 2017

Uh oh!

AppVeyorBot commented Mar 2, 2017

Uh oh!

dmah42 commented Mar 2, 2017

Uh oh!

biojppm commented Apr 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 1, 2017

Uh oh!

AppVeyorBot commented Apr 1, 2017

Uh oh!

biojppm commented Apr 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented May 2, 2017

Uh oh!

coveralls commented May 2, 2017

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 May 2, 2017

Uh oh!

AppVeyorBot commented May 2, 2017

Uh oh!

coveralls commented May 2, 2017

Uh oh!

biojppm commented May 2, 2017

Uh oh!

AppVeyorBot commented May 2, 2017

Uh oh!

biojppm commented May 2, 2017

Uh oh!

coveralls commented May 2, 2017

Uh oh!

AppVeyorBot commented May 2, 2017

Uh oh!

AppVeyorBot commented May 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

biojppm commented Apr 1, 2017 •

edited

Loading