diff --git a/src/benchmark_runner.cc b/src/benchmark_runner.cc index f6d37e0172..e1c0ee2ba0 100644 --- a/src/benchmark_runner.cc +++ b/src/benchmark_runner.cc @@ -370,17 +370,36 @@ BenchmarkRunner::IterationResults BenchmarkRunner::DoNIterations() { return i; } +double BenchmarkRunner::GetTimeForDecision(const IterationResults& i) const { + // real_time and manual_time are accumulated per thread in RunInThread (i.e. + // summed across all threads), and so is whole-process CPU time. To decide + // whether each thread has run for at least min_time we need the per-thread + // (wall-clock-equivalent) duration, so divide the summed value by the number + // of threads. Per-thread CPU time (the default) is left untouched. The summed + // values in i.results are kept intact for reporting, where they are divided + // by the total iteration count across all threads. This mirrors the scaling + // that was applied unconditionally before #1836 removed it from the reported + // path. + double seconds = i.seconds; + if (b.use_manual_time() || b.use_real_time() || + b.measure_process_cpu_time()) { + seconds /= b.threads(); + } + return seconds; +} + IterationCount BenchmarkRunner::PredictNumItersNeeded( const IterationResults& i) const { + const double seconds = GetTimeForDecision(i); // See how much iterations should be increased by. // Note: Avoid division by zero with max(seconds, 1ns). - double multiplier = GetMinTimeToApply() * 1.4 / std::max(i.seconds, 1e-9); + double multiplier = GetMinTimeToApply() * 1.4 / std::max(seconds, 1e-9); // If our last run was at least 10% of FLAGS_benchmark_min_time then we // use the multiplier directly. // Otherwise we use at most 10 times expansion. // NOTE: When the last run was at least 10% of the min time the max // expansion should be 14x. - const bool is_significant = (i.seconds / GetMinTimeToApply()) > 0.1; + const bool is_significant = (seconds / GetMinTimeToApply()) > 0.1; multiplier = is_significant ? multiplier : 10.0; // So what seems to be the sufficiently-large iteration count? Round up. @@ -399,15 +418,17 @@ bool BenchmarkRunner::ShouldReportIterationResults( // Determine if this run should be reported; // Either it has run for a sufficient amount of time // or because an error was reported. + const double seconds = GetTimeForDecision(i); + // real_time_used is accumulated across all threads; use the per-thread value + // so the guard below scales the same way regardless of thread count. + const double real_time_used = i.results.real_time_used / b.threads(); return (i.results.skipped_ != 0u) || FLAGS_benchmark_dry_run || - i.iters >= kMaxIterations || // Too many iterations already. - i.seconds >= - GetMinTimeToApply() || // The elapsed time is large enough. + i.iters >= kMaxIterations || // Too many iterations already. + seconds >= GetMinTimeToApply() || // The elapsed time is large enough. // CPU time is specified but the elapsed real time greatly exceeds // the minimum time. // Note that user provided timers are except from this test. - ((i.results.real_time_used >= 5 * GetMinTimeToApply()) && - !b.use_manual_time()); + ((real_time_used >= 5 * GetMinTimeToApply()) && !b.use_manual_time()); } double BenchmarkRunner::GetMinTimeToApply() const { diff --git a/src/benchmark_runner.h b/src/benchmark_runner.h index 9a2231a2a4..48bc8d9d6b 100644 --- a/src/benchmark_runner.h +++ b/src/benchmark_runner.h @@ -113,6 +113,11 @@ class BenchmarkRunner { bool ShouldReportIterationResults(const IterationResults& i) const; + // Returns the elapsed time, in seconds, that the min-time stopping decision + // should be based on. real/manual time (and whole-process CPU time) are + // accumulated across all threads, so this returns the per-thread value. + double GetTimeForDecision(const IterationResults& i) const; + double GetMinTimeToApply() const; void FinishWarmUp(const IterationCount& i); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index fe88841dd9..b1d32f9973 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -259,6 +259,7 @@ if (BENCHMARK_ENABLE_GTEST_TESTS) add_gtest(reporter_list_gtest) add_gtest(time_unit_gtest) add_gtest(min_time_parse_gtest) + add_gtest(min_time_threads_gtest) add_gtest(profiler_manager_gtest) add_gtest(benchmark_setup_teardown_cb_types_gtest) add_gtest(memory_results_gtest) diff --git a/test/min_time_threads_gtest.cc b/test/min_time_threads_gtest.cc new file mode 100644 index 0000000000..a9e07cf2fd --- /dev/null +++ b/test/min_time_threads_gtest.cc @@ -0,0 +1,83 @@ +#include +#include +#include + +#include "benchmark/benchmark.h" +#include "gtest/gtest.h" + +// Regression test for #2117: the --benchmark_min_time budget must be respected +// *per thread*. Because real/manual time (and whole-process CPU time) are +// accumulated across all threads, the min-time stopping decision has to divide +// that accumulated time by the thread count. Otherwise a benchmark that uses +// N threads stops after only min_time/N of wall-clock time per thread. + +namespace { + +using benchmark::Benchmark; +using benchmark::ClearRegisteredBenchmarks; +using benchmark::ConsoleReporter; +using benchmark::RegisterBenchmark; +using benchmark::RunSpecifiedBenchmarks; +using benchmark::State; + +constexpr double kMinTime = 0.1; // seconds, matches the flag set below. + +void BM_ThreadedSleep(State& state) { + for (auto _ : state) { + std::this_thread::sleep_for(std::chrono::microseconds(200)); + } +} + +class TestReporter : public ConsoleReporter { + public: + bool ReportContext(const Context& /*unused*/) override { return true; } + void PrintHeader(const Run&) override {} + void PrintRunData(const Run& run) override { + // Ignore aggregate rows (mean/median/stddev), keep the real runs. + if (run.repetition_index < 0) return; + runs.push_back(run); + } + + // Wall-clock time a single thread spent in the run with `threads` threads. + double PerThreadWallTime(int threads) const { + for (const auto& run : runs) { + if (run.threads == threads) { + // real_accumulated_time is summed across all threads. + return run.real_accumulated_time / static_cast(run.threads); + } + } + return -1.0; + } + + std::vector runs; +}; + +} // namespace + +TEST(MinTimeThreadsTest, MinTimeRespectedPerThread) { + RegisterBenchmark("BM_ThreadedSleep", BM_ThreadedSleep) + ->UseRealTime() + ->Threads(1) + ->Threads(4); + + const char* argv[] = {"min_time_threads_gtest", "--benchmark_min_time=0.1s"}; + int argc = 2; + benchmark::Initialize(&argc, const_cast(argv)); + + TestReporter reporter; + RunSpecifiedBenchmarks(&reporter, "BM_ThreadedSleep"); + ClearRegisteredBenchmarks(); + + const double wall_1 = reporter.PerThreadWallTime(1); + const double wall_4 = reporter.PerThreadWallTime(4); + ASSERT_GT(wall_1, 0.0) << "single-threaded run missing"; + ASSERT_GT(wall_4, 0.0) << "4-threaded run missing"; + + // Each thread should run for roughly min_time, independent of thread count. + EXPECT_GE(wall_1, 0.7 * kMinTime); + // The core assertion: with the bug the 4-thread run stops after ~min_time/4 + // per thread, so this would be ~0.25 * wall_1 instead of ~wall_1. + EXPECT_GE(wall_4, 0.7 * wall_1) + << "per-thread wall time collapsed with more threads: 1-thread=" << wall_1 + << "s 4-thread=" << wall_4 << "s"; +}