diff --git a/src/benchmark_runner.cc b/src/benchmark_runner.cc index f6d37e017..4046a8deb 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 280113cce..638f03b49 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -261,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 000000000..adf6bed93 --- /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