From 611bbefd8da63bf33fadb9c7a2dc8b9e080c65bc Mon Sep 17 00:00:00 2001 From: devtejasx Date: Mon, 13 Jul 2026 22:11:40 +0530 Subject: [PATCH 1/2] Exclude setup/teardown from MemoryManager measurement window (#2149) RunMemoryManager called memory_manager->Start() before allocating the internal ThreadManager and running the benchmark's Setup(), and called Stop() after resetting the ThreadManager and running Teardown(). As a result the MemoryManager attributed the library's own bookkeeping allocation and any per-benchmark fixture allocations to the benchmark, so the reported allocation count could never be accurate. Setup()/Teardown() already run outside the timed region; make memory measurement consistent by bracketing only RunInThread with Start()/ Stop(), and move the ThreadManager creation and Setup()/Teardown() outside that window. Add memory_manager_ordering_test, which registers a MemoryManager and asserts no Setup/Teardown occurs between Start() and Stop(). It fails on the previous ordering and passes with this change. Signed-off-by: devtejasx --- src/benchmark_runner.cc | 12 +++-- test/CMakeLists.txt | 3 ++ test/memory_manager_ordering_test.cc | 70 ++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 3 deletions(-) create mode 100644 test/memory_manager_ordering_test.cc diff --git a/src/benchmark_runner.cc b/src/benchmark_runner.cc index f6d37e0172..4046a8deb2 100644 --- a/src/benchmark_runner.cc +++ b/src/benchmark_runner.cc @@ -462,17 +462,23 @@ void BenchmarkRunner::RunWarmUp() { MemoryManager::Result BenchmarkRunner::RunMemoryManager( IterationCount memory_iterations) { - memory_manager->Start(); + // Set up the thread manager and run the user's Setup() outside the + // measured window. These happen outside the timed region too, so the + // library's own ThreadManager allocation and any per-benchmark fixture + // allocations must not be attributed to the benchmark's memory usage. std::unique_ptr manager; manager.reset(new internal::ThreadManager(1)); b.Setup(); + + memory_manager->Start(); RunInThread(&b, memory_iterations, 0, manager.get(), perf_counters_measurement_ptr, /*profiler_manager=*/nullptr); - manager.reset(); - b.Teardown(); MemoryManager::Result memory_result; memory_manager->Stop(memory_result); + + manager.reset(); + b.Teardown(); memory_result.memory_iterations = memory_iterations; return memory_result; } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 280113ccea..759b1e36ae 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -97,6 +97,9 @@ benchmark_add_test(NAME spec_arg_verbosity COMMAND spec_arg_verbosity_test --v=4 compile_benchmark_test(benchmark_setup_teardown_test) benchmark_add_test(NAME benchmark_setup_teardown COMMAND benchmark_setup_teardown_test) +compile_benchmark_test(memory_manager_ordering_test) +benchmark_add_test(NAME memory_manager_ordering COMMAND memory_manager_ordering_test) + compile_benchmark_test(filter_test) macro(add_filter_test name filter expect) benchmark_add_test(NAME ${name} COMMAND filter_test --benchmark_min_time=0.01s --benchmark_filter=${filter} ${expect}) diff --git a/test/memory_manager_ordering_test.cc b/test/memory_manager_ordering_test.cc new file mode 100644 index 0000000000..02c22d0e5a --- /dev/null +++ b/test/memory_manager_ordering_test.cc @@ -0,0 +1,70 @@ +// Verifies that a registered MemoryManager only brackets the benchmark's +// measured work: the library's own ThreadManager allocation and the +// per-benchmark Setup()/Teardown() must run outside the Start()/Stop() +// window, consistent with how they run outside the timed region. See +// https://github.com/google/benchmark/issues/2149. + +#include +#include +#include +#include + +#include "benchmark/benchmark.h" + +namespace { + +// Ordered log of lifecycle events. All of these callbacks run on the main +// thread (thread_index 0), so no synchronization is needed. +std::vector events; + +class OrderingMemoryManager : public benchmark::MemoryManager { + public: + void Start() override { events.emplace_back("Start"); } + void Stop(Result& result) override { + events.emplace_back("Stop"); + result.num_allocs = 0; + result.max_bytes_used = 0; + } +}; + +void DoSetup(const benchmark::State&) { events.emplace_back("Setup"); } +void DoTeardown(const benchmark::State&) { events.emplace_back("Teardown"); } + +void BM_ordering(benchmark::State& state) { + for (auto _ : state) { + } +} +BENCHMARK(BM_ordering)->Iterations(1)->Setup(DoSetup)->Teardown(DoTeardown); + +} // namespace + +int main(int argc, char** argv) { + benchmark::MaybeReenterWithoutASLR(argc, argv); + + OrderingMemoryManager mm; + benchmark::RegisterMemoryManager(&mm); + + benchmark::Initialize(&argc, argv); + const size_t ret = benchmark::RunSpecifiedBenchmarks("."); + assert(ret > 0); + + benchmark::RegisterMemoryManager(nullptr); + + // Exactly one memory-measurement pass should have run. + auto start = std::find(events.begin(), events.end(), "Start"); + auto stop = std::find(events.begin(), events.end(), "Stop"); + assert(start != events.end() && "MemoryManager::Start was never called"); + assert(stop != events.end() && "MemoryManager::Stop was never called"); + assert(start < stop && "Start must precede Stop"); + + // Nothing between Start and Stop may be a Setup or Teardown: those run + // outside the measured window (this is what #2149 fixes). + for (auto it = start + 1; it != stop; ++it) { + assert(*it != "Setup" && + "Setup ran inside the MemoryManager Start/Stop window"); + assert(*it != "Teardown" && + "Teardown ran inside the MemoryManager Start/Stop window"); + } + + return 0; +} From 3a4023ddf080136d5932b9e9633f79a56ade10b4 Mon Sep 17 00:00:00 2001 From: devtejasx Date: Thu, 16 Jul 2026 01:59:15 +0530 Subject: [PATCH 2/2] test: convert MemoryManager ordering test to a gtest Address review feedback on #2247: replace the assert-based memory_manager_ordering_test with a gtest that tracks the Start/Stop measurement window and the fixture callbacks with booleans and checks the invariant inside the callbacks, instead of collecting a string log of lifecycle events. --- test/CMakeLists.txt | 4 +- test/memory_manager_ordering_gtest.cc | 79 +++++++++++++++++++++++++++ test/memory_manager_ordering_test.cc | 70 ------------------------ 3 files changed, 80 insertions(+), 73 deletions(-) create mode 100644 test/memory_manager_ordering_gtest.cc delete mode 100644 test/memory_manager_ordering_test.cc diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 759b1e36ae..638f03b49e 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -97,9 +97,6 @@ benchmark_add_test(NAME spec_arg_verbosity COMMAND spec_arg_verbosity_test --v=4 compile_benchmark_test(benchmark_setup_teardown_test) benchmark_add_test(NAME benchmark_setup_teardown COMMAND benchmark_setup_teardown_test) -compile_benchmark_test(memory_manager_ordering_test) -benchmark_add_test(NAME memory_manager_ordering COMMAND memory_manager_ordering_test) - compile_benchmark_test(filter_test) macro(add_filter_test name filter expect) benchmark_add_test(NAME ${name} COMMAND filter_test --benchmark_min_time=0.01s --benchmark_filter=${filter} ${expect}) @@ -264,6 +261,7 @@ if (BENCHMARK_ENABLE_GTEST_TESTS) add_gtest(profiler_manager_gtest) add_gtest(benchmark_setup_teardown_cb_types_gtest) add_gtest(memory_results_gtest) + add_gtest(memory_manager_ordering_gtest) endif(BENCHMARK_ENABLE_GTEST_TESTS) ############################################################################### diff --git a/test/memory_manager_ordering_gtest.cc b/test/memory_manager_ordering_gtest.cc new file mode 100644 index 0000000000..adf6bed938 --- /dev/null +++ b/test/memory_manager_ordering_gtest.cc @@ -0,0 +1,79 @@ +// Verifies that a registered MemoryManager only brackets the benchmark's +// measured work: the per-benchmark Setup()/Teardown() must run outside the +// Start()/Stop() window, consistent with how they run outside the timed +// region. See https://github.com/google/benchmark/issues/2149. + +#include + +#include "benchmark/benchmark.h" +#include "gtest/gtest.h" + +namespace benchmark { +namespace { + +// True while the MemoryManager measurement window is open, i.e. between +// MemoryManager::Start() and Stop(). The fixture callbacks must never observe +// it as true. Everything here runs on the main thread (thread_index 0), so no +// synchronization is needed. +bool in_measurement_window = false; + +// Set from the callbacks so the test fails loudly rather than passing +// vacuously if a code path is never exercised. +bool memory_manager_ran = false; +bool setup_ran = false; +bool teardown_ran = false; + +class OrderingMemoryManager : public MemoryManager { + public: + void Start() override { + in_measurement_window = true; + memory_manager_ran = true; + } + void Stop(Result& result) override { + in_measurement_window = false; + result.num_allocs = 0; + result.max_bytes_used = 0; + } +}; + +void DoSetup(const State&) { + EXPECT_FALSE(in_measurement_window) + << "Setup ran inside the MemoryManager Start/Stop window"; + setup_ran = true; +} +void DoTeardown(const State&) { + EXPECT_FALSE(in_measurement_window) + << "Teardown ran inside the MemoryManager Start/Stop window"; + teardown_ran = true; +} + +void BM_ordering(State& state) { + for (auto _ : state) { + } +} +BENCHMARK(BM_ordering)->Iterations(1)->Setup(DoSetup)->Teardown(DoTeardown); + +// Swallows reporter output so the benchmark run does not pollute test output. +class NullReporter : public BenchmarkReporter { + public: + bool ReportContext(const Context&) override { return true; } + void ReportRuns(const std::vector&) override {} +}; + +} // namespace + +TEST(MemoryManagerOrdering, SetupTeardownRunOutsideMeasurementWindow) { + OrderingMemoryManager mm; + RegisterMemoryManager(&mm); + NullReporter reporter; + const size_t ran = RunSpecifiedBenchmarks(&reporter); + RegisterMemoryManager(nullptr); + + EXPECT_GT(ran, 0u); + 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_FALSE(in_measurement_window); +} + +} // namespace benchmark diff --git a/test/memory_manager_ordering_test.cc b/test/memory_manager_ordering_test.cc deleted file mode 100644 index 02c22d0e5a..0000000000 --- a/test/memory_manager_ordering_test.cc +++ /dev/null @@ -1,70 +0,0 @@ -// Verifies that a registered MemoryManager only brackets the benchmark's -// measured work: the library's own ThreadManager allocation and the -// per-benchmark Setup()/Teardown() must run outside the Start()/Stop() -// window, consistent with how they run outside the timed region. See -// https://github.com/google/benchmark/issues/2149. - -#include -#include -#include -#include - -#include "benchmark/benchmark.h" - -namespace { - -// Ordered log of lifecycle events. All of these callbacks run on the main -// thread (thread_index 0), so no synchronization is needed. -std::vector events; - -class OrderingMemoryManager : public benchmark::MemoryManager { - public: - void Start() override { events.emplace_back("Start"); } - void Stop(Result& result) override { - events.emplace_back("Stop"); - result.num_allocs = 0; - result.max_bytes_used = 0; - } -}; - -void DoSetup(const benchmark::State&) { events.emplace_back("Setup"); } -void DoTeardown(const benchmark::State&) { events.emplace_back("Teardown"); } - -void BM_ordering(benchmark::State& state) { - for (auto _ : state) { - } -} -BENCHMARK(BM_ordering)->Iterations(1)->Setup(DoSetup)->Teardown(DoTeardown); - -} // namespace - -int main(int argc, char** argv) { - benchmark::MaybeReenterWithoutASLR(argc, argv); - - OrderingMemoryManager mm; - benchmark::RegisterMemoryManager(&mm); - - benchmark::Initialize(&argc, argv); - const size_t ret = benchmark::RunSpecifiedBenchmarks("."); - assert(ret > 0); - - benchmark::RegisterMemoryManager(nullptr); - - // Exactly one memory-measurement pass should have run. - auto start = std::find(events.begin(), events.end(), "Start"); - auto stop = std::find(events.begin(), events.end(), "Stop"); - assert(start != events.end() && "MemoryManager::Start was never called"); - assert(stop != events.end() && "MemoryManager::Stop was never called"); - assert(start < stop && "Start must precede Stop"); - - // Nothing between Start and Stop may be a Setup or Teardown: those run - // outside the measured window (this is what #2149 fixes). - for (auto it = start + 1; it != stop; ++it) { - assert(*it != "Setup" && - "Setup ran inside the MemoryManager Start/Stop window"); - assert(*it != "Teardown" && - "Teardown ran inside the MemoryManager Start/Stop window"); - } - - return 0; -}