Exclude setup/teardown from the MemoryManager measurement window (#2149) (#2276)

RunMemoryManager() opened the MemoryManager window before allocating the
ThreadManager and running the per-benchmark Setup(), and closed it after
Teardown(). Everything the library and the fixture allocate around the
benchmark was therefore attributed to the benchmark itself, so an exact
allocation count was impossible to obtain.

Move Start()/Stop() to bracket only RunInThread(), which is the same region
the timers cover. Setup()/Teardown() already run outside the timed region, so
this makes the memory window consistent with the time window.

Add memory_manager_ordering_gtest, which registers a MemoryManager that
records whether the window is open and asserts the fixture callbacks never
observe it as open. It fails on the previous ordering ("Teardown ran inside
the MemoryManager Start/Stop window") and passes with this change.

src/benchmark_runner.cc

@@ -462,17 +462,17 @@ void BenchmarkRunner::RunWarmUp() {
 
 MemoryManager::Result BenchmarkRunner::RunMemoryManager(
     IterationCount memory_iterations) {
-  memory_manager->Start();
   std::unique_ptr<internal::ThreadManager> 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;
 }

test/CMakeLists.txt

@@ -262,6 +262,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)
 
 ###############################################################################

test/memory_manager_ordering_gtest.cc

@@ -0,0 +1,74 @@
+// Setup()/Teardown() must run outside the MemoryManager Start()/Stop()
+// window, as they already do outside the timed region. See #2149.
+
+#include <vector>
+
+#include "benchmark/benchmark.h"
+#include "gtest/gtest.h"
+
+namespace benchmark {
+namespace {
+
+// Everything below runs on the main thread, so no synchronization is needed.
+bool in_measurement_window = false;
+
+// Recorded so the test fails rather than passes vacuously when a code path is
+// never reached.
+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.
+class NullReporter : public BenchmarkReporter {
+ public:
+  bool ReportContext(const Context&) override { return true; }
+  void ReportRuns(const std::vector<Run>&) 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