[NFC] Refactor RunBenchmark() #690
Conversation
|
❌ Build benchmark 1470 failed (commit 93720b92ec by @LebedevRI) |
|
❌ Build benchmark 1471 failed (commit b7ef961139 by @LebedevRI) |
src/benchmark.cc
Outdated
| } | ||
|
|
||
| return run_results; | ||
| RunResults getResults() { return run_results; } |
There was a problem hiding this comment.
I'm torn between this, where you have a simple accessor but the constructor is complex and actually doing the work, and an alternative where the constructor merely initialises the state, and a method Run that runs the benchmark and returns RunResults.
There was a problem hiding this comment.
Yes, i don't like this, too.
The current approach (complex constructor) seems better,
because we really wouldn't want to re-run re-use
the same BenchmarkRunner instance more than once.
|
This seems to work really well. I wonder if it's worth extracting the class into a benchmark_runner.cc implementation file to help reduce the overwhelming benchmark.cc file size. |
Well, it should be the same algo, just less, convoluted?
Done, better? |
|
❌ Build benchmark 1487 failed (commit c356a2f34d by @LebedevRI) |
|
Anything more to do here? From my side, all the follow-up work in |
LebedevRI
force-pushed
the
refactor-RunBenchmark
branch
from
7406c89
to
1267547
Compare
|
❌ Build benchmark 1494 failed (commit c7a3c9277c by @LebedevRI) |
|
✅ Build benchmark 1495 completed (commit fdd6be7f6c by @LebedevRI) |
src/benchmark_runner.cc
Outdated
| } | ||
| } | ||
|
|
||
| RunResults getResults() { return run_results; } |
There was a problem hiding this comment.
google-style: get_results()
also, mark the method as const?
There was a problem hiding this comment.
I think even the current non-const variant is not what i wanted..
https://godbolt.org/z/7ALmcF
src/benchmark_runner.cc
Outdated
| size_t iters; | ||
| double seconds; | ||
| }; | ||
| IterationResults doNIterations() { |
src/benchmark_runner.cc
Outdated
| return i; | ||
| } | ||
|
|
||
| size_t predictNumItersNeeded(const IterationResults& i) const { |
src/benchmark_runner.cc
Outdated
| return next_iters; // round up before conversion to integer. | ||
| } | ||
|
|
||
| bool shouldReportIterationResults(const IterationResults& i) const { |
src/benchmark_runner.cc
Outdated
| ((i.results.real_time_used >= 5 * min_time) && !b.use_manual_time); | ||
| } | ||
|
|
||
| void doOneRepetition(bool is_the_first_repetition) { |
| @@ -170,7 +170,7 @@ class BenchmarkRunner { | |||
| } | |||
| } | |||
|
|
|||
| RunResults getResults() { return run_results; } | |||
| RunResults&& get_results() { return std::move(run_results); } | |||
There was a problem hiding this comment.
nice catch :) that's much better.
|
Thank you for the review! |
|
✅ Build benchmark 1504 completed (commit ac305f101c by @LebedevRI) |
Ok, so, i'm still trying to get to the state when it will be a trivial change to report all the separate iterations. The old code (LHS of the diff) was rather convoluted i'd say. I have tried to refactor it a bit into *small* logical chunks, with proper comments. As far as i can tell, i preserved the intent of the code, what it was doing before. The road forward still isn't clear, but i'm quite sure it's not with the old code :)
Ok, so, i'm still trying to get to the state when it will be a trivial change to report all the separate iterations.
The old code (LHS of the diff) was rather convoluted i'd say.
I have tried to refactor it a bit into small logical chunks, with proper comments.
As far as i can tell, i preserved the intent of the code, what it was doing before.
The road forward still isn't clear, but i'm quite sure it's not with the old code :)