Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the initialization of min_warmup_time in BenchmarkRunner by removing the check on b.min_time(), allowing warmup time to be configured independently of the minimum benchmark run time. It also adds a comprehensive suite of unit tests in test/benchmark_warmup_gtest.cc to verify warmup behavior under various configurations (such as local/global warmup overrides and dry runs) and registers this new test in test/CMakeLists.txt. There are no review comments to address, and I have no additional feedback to provide.
|
Hmm yeah, looking through #1399, it is not apparent to me why warmup should only be done when the benchmark iteself specifies its own min time. |
LebedevRI
left a comment
LebedevRI
left a comment
There was a problem hiding this comment.
Does this PR need an AI disclaimer?
Nope, I only wrote the PR description in Turkish and used an LLM to translate it into English. |
| namespace benchmark { | ||
|
|
||
| BM_DECLARE_bool(benchmark_dry_run); | ||
| BM_DECLARE_string(benchmark_min_time); | ||
| BM_DECLARE_double(benchmark_min_warmup_time); |
There was a problem hiding this comment.
I think i'm not entirely happy with this testing strategy :(
There was a problem hiding this comment.
Could you elaborate on what concerns you about the testing strategy? I'd be happy to revise the tests accordingly.
There was a problem hiding this comment.
The entirety of this approach, i'm afraid. I don't like how this needs to open benchmark namespace.
There is no nice way to test this, the alternative is running the test in a subprocess...
IMHO this is the case where i'd be grudgingly okay with not having test coverage.
There was a problem hiding this comment.
Understood, thanks for clarifying it!
I will remove the tests and keep the PR focused on the fix itself.
|
I've removed the regression tests as suggested. Thanks for the feedback! |
LebedevRI
left a comment
LebedevRI
left a comment
There was a problem hiding this comment.
So i think this is fine. I can't understand why that condition was there,
the original PR does not seem to explain it.
This branch has not been deployed
2 participants
Fixed a bug where
MinWarmUpTime()didn't work unlessMinTime()was also set.The fix allows benchmarks to use their own warmup time without requiring
MinTime(), while keeping the existing global warmup and dry-run behavior.I also added six tests to cover different warmup settings.
Testing:
Assembly tests were skipped because they already failed on the original code with GCC 16.2.1.
No API or ABI changes.