[NFC] BenchmarkRunner: always populate *_report_aggregates_only bools. #708
Conversation
It is better to let the RunBenchmarks(), report() decide whether to actually *only* output aggregates or not, depending on whether there are actually aggregates.
|
✅ Build benchmark 1532 completed (commit 823fb4aa64 by @LebedevRI) |
|
I'm missing the subtlety here. why is it ok to remove the results count check? |
|
It's subtle indeed. Previously, Now, the logic is the same, but the blame has shifted. To put it in the context of the patch series - if the repetition count was Did that make sense? |
|
It's basically the same problem as the cause for the #707 rename. |
|
Thank you for the review! |
google#708) It is better to let the RunBenchmarks(), report() decide whether to actually *only* output aggregates or not, depending on whether there are actually aggregates. It's subtle indeed. Previously, `BenchmarkRunner()` always said that "if there are no repetitions, then you should never output only the repetitions". And the `report()` simply assumed that the `report_aggregates_only` bool it received makes sense, and simply used it. Now, the logic is the same, but the blame has shifted. `BenchmarkRunner()` always propagates what those benchmarks would have wanted to happen wrt the aggregates. And the `report()` lambda has to actually consider both the `report_aggregates_only` bool, and it's meaningfulness. To put it in the context of the patch series - if the repetition count was `1`, but `*_report_aggregates_only` was set to `true`, and we capture each iteration separately, then we will compute the aggregates, but then output everything, both the iteration, and aggregates, despite `*_report_aggregates_only` being set to `true`.
google#708) It is better to let the RunBenchmarks(), report() decide whether to actually *only* output aggregates or not, depending on whether there are actually aggregates. It's subtle indeed. Previously, `BenchmarkRunner()` always said that "if there are no repetitions, then you should never output only the repetitions". And the `report()` simply assumed that the `report_aggregates_only` bool it received makes sense, and simply used it. Now, the logic is the same, but the blame has shifted. `BenchmarkRunner()` always propagates what those benchmarks would have wanted to happen wrt the aggregates. And the `report()` lambda has to actually consider both the `report_aggregates_only` bool, and it's meaningfulness. To put it in the context of the patch series - if the repetition count was `1`, but `*_report_aggregates_only` was set to `true`, and we capture each iteration separately, then we will compute the aggregates, but then output everything, both the iteration, and aggregates, despite `*_report_aggregates_only` being set to `true`.
It is better to let the RunBenchmarks(), report() decide
whether to actually only output aggregates or not,
depending on whether there are actually aggregates.