From 2b36d89de1af9325b6f2e1eb5f765d10264bdffc Mon Sep 17 00:00:00 2001 From: ReturnKartikey Date: Thu, 8 Oct 2026 01:40:28 +0530 Subject: [PATCH] fix: report threads as 1 in State during single-threaded manager passes (fixes #1849) In BenchmarkRunner::RunMemoryManager and BenchmarkRunner::RunProfilerManager, execution deliberately runs single-threaded with ThreadManager(1) and thread 0. However, State was constructed with the benchmark's configured thread count (b.threads()), causing state.threads() to report multiple threads despite only a single thread running. For multithreaded benchmarks and fixtures that synchronize or coordinate across state.threads() (such as barriers, latches, or SetUp/TearDown hooks), thread 0 hung waiting indefinitely for other threads that were never spawned. This patch updates BenchmarkInstance::Run, Setup, and Teardown to accept an optional thread_count override and passes thread_count = 1 during RunMemoryManager and RunProfilerManager passes so state.threads() correctly reflects the active single-threaded execution. Also adds a regression test in memory_manager_ordering_gtest. --- src/benchmark_api_internal.cc | 19 ++++---- src/benchmark_api_internal.h | 6 +-- src/benchmark_runner.cc | 18 ++++---- test/memory_manager_ordering_gtest.cc | 62 +++++++++++++++++++++++++-- 4 files changed, 83 insertions(+), 22 deletions(-) diff --git a/src/benchmark_api_internal.cc b/src/benchmark_api_internal.cc index f9c4990dd..53bd0f42d 100644 --- a/src/benchmark_api_internal.cc +++ b/src/benchmark_api_internal.cc @@ -93,25 +93,28 @@ State BenchmarkInstance::Run( IterationCount iters, int thread_id, internal::ThreadTimer* timer, internal::ThreadManager* manager, internal::PerfCountersMeasurement* perf_counters_measurement, - ProfilerManager* profiler_manager) const { - State st(name_.function_name, iters, args_, thread_id, threads_, timer, + ProfilerManager* profiler_manager, int thread_count) const { + const int n_threads = thread_count > 0 ? thread_count : threads_; + State st(name_.function_name, iters, args_, thread_id, n_threads, timer, manager, perf_counters_measurement, profiler_manager); benchmark_.Run(st); return st; } -void BenchmarkInstance::Setup() const { +void BenchmarkInstance::Setup(int thread_count) const { if (setup_ != nullptr) { - State st(name_.function_name, /*iters*/ 1, args_, /*thread_id*/ 0, threads_, - nullptr, nullptr, nullptr, nullptr); + const int n_threads = thread_count > 0 ? thread_count : threads_; + State st(name_.function_name, /*iters*/ 1, args_, /*thread_id*/ 0, + n_threads, nullptr, nullptr, nullptr, nullptr); setup_(st); } } -void BenchmarkInstance::Teardown() const { +void BenchmarkInstance::Teardown(int thread_count) const { if (teardown_ != nullptr) { - State st(name_.function_name, /*iters*/ 1, args_, /*thread_id*/ 0, threads_, - nullptr, nullptr, nullptr, nullptr); + const int n_threads = thread_count > 0 ? thread_count : threads_; + State st(name_.function_name, /*iters*/ 1, args_, /*thread_id*/ 0, + n_threads, nullptr, nullptr, nullptr, nullptr); teardown_(st); } } diff --git a/src/benchmark_api_internal.h b/src/benchmark_api_internal.h index 0dd950cbe..81b2b1ec4 100644 --- a/src/benchmark_api_internal.h +++ b/src/benchmark_api_internal.h @@ -41,8 +41,8 @@ class BenchmarkInstance { double min_warmup_time() const { return min_warmup_time_; } IterationCount iterations() const { return iterations_; } int threads() const { return threads_; } - void Setup() const; - void Teardown() const; + void Setup(int thread_count = -1) const; + void Teardown(int thread_count = -1) const; const auto& GetUserThreadRunnerFactory() const { return benchmark_.threadrunner_; } @@ -50,7 +50,7 @@ class BenchmarkInstance { State Run(IterationCount iters, int thread_id, internal::ThreadTimer* timer, internal::ThreadManager* manager, internal::PerfCountersMeasurement* perf_counters_measurement, - ProfilerManager* profiler_manager) const; + ProfilerManager* profiler_manager, int thread_count = -1) const; private: BenchmarkName name_; diff --git a/src/benchmark_runner.cc b/src/benchmark_runner.cc index 0d267a38d..a10d5a808 100644 --- a/src/benchmark_runner.cc +++ b/src/benchmark_runner.cc @@ -144,14 +144,14 @@ BenchmarkReporter::Run CreateRunReport( void RunInThread(const BenchmarkInstance* b, IterationCount iters, int thread_id, ThreadManager* manager, PerfCountersMeasurement* perf_counters_measurement, - ProfilerManager* profiler_manager_) { + ProfilerManager* profiler_manager_, int thread_count = -1) { internal::ThreadTimer timer( b->measure_process_cpu_time() ? internal::ThreadTimer::CreateProcessCpuTime() : internal::ThreadTimer::Create()); State st = b->Run(iters, thread_id, &timer, manager, - perf_counters_measurement, profiler_manager_); + perf_counters_measurement, profiler_manager_, thread_count); if (!(st.skipped() || st.iterations() >= st.max_iterations)) { st.SkipWithError( "The benchmark didn't run, nor was it explicitly skipped. Please call " @@ -464,15 +464,16 @@ MemoryManager::Result BenchmarkRunner::RunMemoryManager( IterationCount memory_iterations) { std::unique_ptr manager; manager.reset(new internal::ThreadManager(1)); - b.Setup(); + b.Setup(/*thread_count=*/1); memory_manager->Start(); RunInThread(&b, memory_iterations, 0, manager.get(), perf_counters_measurement_ptr, - /*profiler_manager=*/nullptr); + /*profiler_manager=*/nullptr, + /*thread_count=*/1); MemoryManager::Result memory_result; memory_manager->Stop(memory_result); manager.reset(); - b.Teardown(); + b.Teardown(/*thread_count=*/1); memory_result.memory_iterations = memory_iterations; return memory_result; } @@ -480,12 +481,13 @@ MemoryManager::Result BenchmarkRunner::RunMemoryManager( void BenchmarkRunner::RunProfilerManager(IterationCount profile_iterations) { std::unique_ptr manager; manager.reset(new internal::ThreadManager(1)); - b.Setup(); + b.Setup(/*thread_count=*/1); RunInThread(&b, profile_iterations, 0, manager.get(), /*perf_counters_measurement_ptr=*/nullptr, - /*profiler_manager=*/profiler_manager); + /*profiler_manager=*/profiler_manager, + /*thread_count=*/1); manager.reset(); - b.Teardown(); + b.Teardown(/*thread_count=*/1); } void BenchmarkRunner::DoOneRepetition() { diff --git a/test/memory_manager_ordering_gtest.cc b/test/memory_manager_ordering_gtest.cc index 14f0de7d9..39360eca6 100644 --- a/test/memory_manager_ordering_gtest.cc +++ b/test/memory_manager_ordering_gtest.cc @@ -1,6 +1,6 @@ -// Setup()/Teardown() must run outside the MemoryManager Start()/Stop() -// window, as they already do outside the timed region. See #2149. - +#include +#include +#include #include #include "benchmark/benchmark.h" @@ -48,6 +48,58 @@ void BM_ordering(State& state) { } BENCHMARK(BM_ordering)->Iterations(1)->Setup(DoSetup)->Teardown(DoTeardown); +// Regression test for #1849: multithreaded benchmarks with memory manager +// run single-threaded during the memory manager pass, and state.threads() must +// report 1 (not the benchmark's configured thread count) to prevent hangs on +// barrier synchronization. +bool memory_pass_threads_is_one = false; +bool benchmark_pass_threads_is_four = false; + +class MultithreadedOrderingFixture : public Fixture { + public: + void SetUp(const State& state) override { Sync(state.threads()); } + + void TearDown(const State& state) override { Sync(state.threads()); } + + private: + void Sync(int thread_count) { + std::unique_lock lock(mutex_); + int gen = generation_; + if (++arrived_ == thread_count) { + arrived_ = 0; + ++generation_; + cv_.notify_all(); + } else { + ASSERT_TRUE(cv_.wait_for(lock, std::chrono::seconds(5), [&] { + return generation_ != gen; + })) << "Timed out waiting for all threads; state.threads() was wrong"; + } + } + + std::mutex mutex_; + std::condition_variable cv_; + int arrived_ = 0; + int generation_ = 0; +}; + +BENCHMARK_DEFINE_F(MultithreadedOrderingFixture, BM_MultithreadedSync) +(State& state) { + if (in_measurement_window) { + if (state.threads() == 1) { + memory_pass_threads_is_one = true; + } + } else { + if (state.threads() == 4) { + benchmark_pass_threads_is_four = true; + } + } + for (auto _ : state) { + } +} +BENCHMARK_REGISTER_F(MultithreadedOrderingFixture, BM_MultithreadedSync) + ->Iterations(1) + ->Threads(4); + // Swallows reporter output. class NullReporter : public BenchmarkReporter { public: @@ -68,6 +120,10 @@ TEST(MemoryManagerOrdering, SetupTeardownRunOutsideMeasurementWindow) { EXPECT_TRUE(memory_manager_ran) << "MemoryManager measurement pass never ran"; EXPECT_TRUE(setup_ran) << "Setup callback never ran"; EXPECT_TRUE(teardown_ran) << "Teardown callback never ran"; + EXPECT_TRUE(memory_pass_threads_is_one) + << "state.threads() was not 1 during MemoryManager measurement pass"; + EXPECT_TRUE(benchmark_pass_threads_is_four) + << "state.threads() was not 4 during normal iterations"; EXPECT_FALSE(in_measurement_window); }