benchmark icon indicating copy to clipboard operation
benchmark copied to clipboard

[FR] Inconsistent handling of repetitions

Open Lectem opened this issue 5 years ago • 1 comments

Is your feature request related to a problem? Please describe.

There are some differences in how repetitions are handled. For family repetitions (the ones hardcoded in the code with BENCHMARK(bench)->Repetitions(X)), a suffix is added /repeats:X.

https://github.com/google/benchmark/blob/b874e72208b6e21b62287942e5e3b11f6630107f/src/benchmark_register.cc#L206

While if you use the `--benchmark_repetitions=X' parameter you will not get the suffix, which is better ! Since the aggregated values display the number of repetitions in the iterations column, I think it is ok not to have the suffix.

example

-----------------------------------------------------------------------------------------------------
Benchmark                                           Time             CPU   Iterations UserCounters...
-----------------------------------------------------------------------------------------------------
unicode_validate_twitter/repeats:10_mean        22807 ns        22807 ns           10 Gigabytes=27.6907G/s docs=43.848k/s
unicode_validate_twitter/repeats:10_median      22818 ns        22818 ns           10 Gigabytes=27.6763G/s docs=43.8252k/s
unicode_validate_twitter/repeats:10_stddev        122 ns          122 ns           10 Gigabytes=148.649M/s docs=235.384/s
unicode_validate_twitter/repeats:10_max         22936 ns        22935 ns           10 Gigabytes=28.0407G/s docs=44.4023k/s

Could simply be

-----------------------------------------------------------------------------------------------------
Benchmark                                           Time             CPU   Iterations UserCounters...
-----------------------------------------------------------------------------------------------------
unicode_validate_twitter_mean                   22807 ns        22807 ns           10 Gigabytes=27.6907G/s docs=43.848k/s
unicode_validate_twitter_median                 22818 ns        22818 ns           10 Gigabytes=27.6763G/s docs=43.8252k/s
unicode_validate_twitter_stddev                   122 ns          122 ns           10 Gigabytes=148.649M/s docs=235.384/s
unicode_validate_twitter_max                    22936 ns        22935 ns           10 Gigabytes=28.0407G/s docs=44.4023k/s

The main issue with the suffix is that when using the json reporter, you usually do not want this suffix in the name since you already have the number of repetitions in the data. It's also a bit problematic if you want to do performance tracking since if you change the number of repetitions between 2 versions of the code, the two tests will be interpreted as different while only the number of repetitions changed.

Describe the solution you'd like

I think the number of repetitions should not be added as a suffix at all.

Describe alternatives you've considered

Another option would be to not add the suffix to the json reporter name fields, since https://github.com/google/benchmark/commit/f6e96861a373c90ea0c727177fc68d2984e048bb already paved the way for this.

Additional notes

If one of the solutions is ok, I can open a pull request.

Lectem avatar Aug 05 '20 15:08 Lectem

I'm happy to find a solution here.

My only concern with dropping the suffix and relying on the 'Iterations' column is that it may not be immediately clear that 'Iterations' is the sample size for the aggregation.

But i'm quite happy to be convinced that it is obvious/documentable and we should just do it.

dmah42 avatar Aug 17 '20 16:08 dmah42