[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 |
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 { |
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 :)