Repository navigation
Conversation
Documents the current behavior in `BenchmarkRunner::DoOneRepetition()`: when a benchmark calls `SkipWithError()` before the loop, `RunMemoryManager()` still runs (`memory_manager_ran == true`) even though `CreateRunReport()` discards all memory results (`memory_iterations == 0`). A follow-up commit fixes `DoOneRepetition()` to skip `RunMemoryManager()` and flips this assertion. AI-assisted: this change was written with the help of an AI coding tool.
Previously `BenchmarkRunner::DoOneRepetition()` called `RunMemoryManager()` even when the timed run was skipped (`i.results.skipped_ != internal::NotSkipped`), re-running `Setup()`, the benchmark body, `MemoryManager::Start()`/`Stop()`, and `Teardown()` even though `CreateRunReport()` discards all memory results when `report.skipped != 0`. Guard `RunMemoryManager()` on `i.results.skipped_ == internal::NotSkipped`, and update the regression test in `memory_manager_ordering_gtest.cc` to expect that `MemoryManager` does not run. AI-assisted: this change was written with the help of an AI coding tool.
|
Let's merge #2377 first then I'll rebase on top. Sad that I can't use stacked github PRs from my repo into the google repo (stacked PRs have to be within the same repo) |
|
What is the need to do this? What is the practical benefit? |
|
Fair question. TBH, there is not a "need" to do this - it's just an optimization. It should make benchmark running faster if we have some skipped benchmarks. This avoids completely discarded work:
That said, if you think it's not worth the branch, that's fine, happy to revert. I see now that RunProfilerManager right below has the same unguarded behaviour. We could skip that too. |
|
i think this is useful for consistency across the performance and memory runs. |
| memory_result = RunMemoryManager(memory_iterations); | ||
| } | ||
|
|
||
| if (profiler_manager != nullptr) { |
There was a problem hiding this comment.
So what about this one then?
There was a problem hiding this comment.
yes i just saw that in the note above. we should do the same.
There was a problem hiding this comment.
@mhansen do you want to add the profiler manager skip into this too?
No description provided.