fix memory manager result bug

include/benchmark/benchmark.h

@@ -394,7 +394,8 @@ class MemoryManager {
         : num_allocs(0),
           max_bytes_used(0),
           total_allocated_bytes(TombstoneValue),
-          net_heap_growth(TombstoneValue) {}
+          net_heap_growth(TombstoneValue),
+          is_valid(false) {}
 
     // The number of allocations made in total between Start and Stop.
     int64_t num_allocs;
@@ -410,6 +411,8 @@ class MemoryManager {
     // ie., total_allocated_bytes - total_deallocated_bytes.
     // Init'ed to TombstoneValue if metric not available.
     int64_t net_heap_growth;
+
+    bool is_valid;
   };
 
   virtual ~MemoryManager() {}
@@ -1721,7 +1724,6 @@ class BENCHMARK_EXPORT BenchmarkReporter {
           complexity_n(0),
           report_big_o(false),
           report_rms(false),
-          memory_result(NULL),
           allocs_per_iter(0.0) {}
 
     std::string benchmark_name() const;
@@ -1777,7 +1779,7 @@ class BENCHMARK_EXPORT BenchmarkReporter {
     UserCounters counters;
 
     // Memory metrics.
-    const MemoryManager::Result* memory_result;
+    MemoryManager::Result memory_result;
     double allocs_per_iter;
   };
 

src/benchmark_runner.cc

@@ -81,7 +81,7 @@ BenchmarkReporter::Run CreateRunReport(
     const benchmark::internal::BenchmarkInstance& b,
     const internal::ThreadManager::Result& results,
     IterationCount memory_iterations,
-    const MemoryManager::Result* memory_result, double seconds,
+    const MemoryManager::Result& memory_result, double seconds,
     int64_t repetition_index, int64_t repeats) {
   // Create report about this benchmark run.
   BenchmarkReporter::Run report;
@@ -114,11 +114,11 @@ BenchmarkReporter::Run CreateRunReport(
     report.counters = results.counters;
 
     if (memory_iterations > 0) {
-      assert(memory_result != nullptr);
+      assert(memory_result.is_valid);
       report.memory_result = memory_result;
       report.allocs_per_iter =
           memory_iterations != 0
-              ? static_cast<double>(memory_result->num_allocs) /
+              ? static_cast<double>(memory_result.num_allocs) /
                     static_cast<double>(memory_iterations)
               : 0;
     }
@@ -426,13 +426,8 @@ void BenchmarkRunner::RunWarmUp() {
   }
 }
 
-MemoryManager::Result* BenchmarkRunner::RunMemoryManager(
+MemoryManager::Result BenchmarkRunner::RunMemoryManager(
     IterationCount memory_iterations) {
-  // TODO(vyng): Consider making BenchmarkReporter::Run::memory_result an
-  // optional so we don't have to own the Result here.
-  // Can't do it now due to cxx03.
-  memory_results.push_back(MemoryManager::Result());
-  MemoryManager::Result* memory_result = &memory_results.back();
   memory_manager->Start();
   std::unique_ptr<internal::ThreadManager> manager;
   manager.reset(new internal::ThreadManager(1));
@@ -443,7 +438,8 @@ MemoryManager::Result* BenchmarkRunner::RunMemoryManager(
   manager->WaitForAllThreads();
   manager.reset();
   b.Teardown();
-  memory_manager->Stop(*memory_result);
+  MemoryManager::Result memory_result;
+  memory_manager->Stop(memory_result);
   return memory_result;
 }
 
@@ -508,13 +504,14 @@ void BenchmarkRunner::DoOneRepetition() {
   }
 
   // Produce memory measurements if requested.
-  MemoryManager::Result* memory_result = nullptr;
+  MemoryManager::Result memory_result;
   IterationCount memory_iterations = 0;
   if (memory_manager != nullptr) {
     // Only run a few iterations to reduce the impact of one-time
     // allocations in benchmarks that are not properly managed.
     memory_iterations = std::min<IterationCount>(16, iters);
     memory_result = RunMemoryManager(memory_iterations);
+    memory_result.is_valid = true;
   }
 
   if (profiler_manager != nullptr) {

src/benchmark_runner.h

@@ -91,8 +91,6 @@ class BenchmarkRunner {
 
   std::vector<std::thread> pool;
 
-  std::vector<MemoryManager::Result> memory_results;
-
   IterationCount iters;  // preserved between repetitions!
   // So only the first repetition has to find/calculate it,
   // the other repetitions will just use that precomputed iteration count.
@@ -106,7 +104,7 @@ class BenchmarkRunner {
   };
   IterationResults DoNIterations();
 
-  MemoryManager::Result* RunMemoryManager(IterationCount memory_iterations);
+  MemoryManager::Result RunMemoryManager(IterationCount memory_iterations);
 
   void RunProfilerManager(IterationCount profile_iterations);
 

src/json_reporter.cc

@@ -306,8 +306,8 @@ void JSONReporter::PrintRunData(Run const& run) {
     out << ",\n" << indent << FormatKV(c.first, c.second);
   }
 
-  if (run.memory_result != nullptr) {
-    const MemoryManager::Result memory_result = *run.memory_result;
+  if (run.memory_result.is_valid) {
+    const MemoryManager::Result& memory_result{run.memory_result};
     out << ",\n" << indent << FormatKV("allocs_per_iter", run.allocs_per_iter);
     out << ",\n"
         << indent << FormatKV("max_bytes_used", memory_result.max_bytes_used);

test/CMakeLists.txt

@@ -233,6 +233,7 @@ if (BENCHMARK_ENABLE_GTEST_TESTS)
   add_gtest(min_time_parse_gtest)
   add_gtest(profiler_manager_gtest)
   add_gtest(benchmark_setup_teardown_cb_types_gtest)
+  add_gtest(memory_results_gtest)
 endif(BENCHMARK_ENABLE_GTEST_TESTS)
 
 ###############################################################################

test/memory_results_gtest.cc

@@ -0,0 +1,102 @@
+#include <vector>
+
+#include "../src/benchmark_runner.h"
+#include "benchmark/benchmark.h"
+#include "gtest/gtest.h"
+
+#define N_REPETITIONS 100
+#define N_ITERATIONS 1
+
+using benchmark::ClearRegisteredBenchmarks;
+using benchmark::ConsoleReporter;
+using benchmark::MemoryManager;
+using benchmark::RegisterBenchmark;
+using benchmark::RunSpecifiedBenchmarks;
+using benchmark::State;
+using benchmark::internal::Benchmark;
+
+namespace counts {
+int num_allocs = 0;
+int max_bytes_used = 0;
+int total_allocated_bytes = 0;
+int net_heap_growth = 0;
+void reset() {
+  num_allocs = 0;
+  max_bytes_used = 0;
+  total_allocated_bytes = 0;
+  net_heap_growth = 0;
+}
+}  // namespace counts
+
+class TestMemoryManager : public MemoryManager {
+  void Start() override {}
+  void Stop(Result& result) override {
+    result.num_allocs = counts::num_allocs;
+    result.net_heap_growth = counts::net_heap_growth;
+    result.max_bytes_used = counts::max_bytes_used;
+    result.total_allocated_bytes = counts::total_allocated_bytes;
+
+    counts::num_allocs += 1;
+    counts::max_bytes_used += 2;
+    counts::net_heap_growth += 4;
+    counts::total_allocated_bytes += 10;
+  }
+};
+
+class TestReporter : public ConsoleReporter {
+ public:
+  TestReporter() = default;
+  virtual ~TestReporter() = default;
+
+  bool ReportContext(const Context& /*unused*/) override { return true; }
+
+  void PrintHeader(const Run&) override {}
+  void PrintRunData(const Run& run) override {
+    if (run.repetition_index == -1) return;
+    if (!run.memory_result.is_valid) return;
+
+    store.push_back(run.memory_result);
+  }
+
+  std::vector<MemoryManager::Result> store;
+};
+
+class MemoryResultsTest : public testing::Test {
+ public:
+  Benchmark* bm;
+  TestReporter reporter;
+
+  void SetUp() override {
+    bm = RegisterBenchmark("BM", [](State& st) {
+      for (auto _ : st) {
+      }
+    });
+    bm->Repetitions(N_REPETITIONS);
+    bm->Iterations(N_ITERATIONS);
+  }
+  void TearDown() override { ClearRegisteredBenchmarks(); }
+};
+
+TEST_F(MemoryResultsTest, NoMMTest) {
+  RunSpecifiedBenchmarks(&reporter);
+  EXPECT_EQ(reporter.store.size(), 0);
+}
+
+TEST_F(MemoryResultsTest, ResultsTest) {
+  counts::reset();
+  MemoryManager* mm = new TestMemoryManager;
+  RegisterMemoryManager(mm);
+  EXPECT_EQ(benchmark::internal::memory_manager, mm);
+
+  RunSpecifiedBenchmarks(&reporter);
+  EXPECT_EQ(reporter.store.size(), N_REPETITIONS);
+
+  for (size_t i = 0; i < reporter.store.size(); i++) {
+    EXPECT_EQ(reporter.store[i].num_allocs, i);
+    EXPECT_EQ(reporter.store[i].max_bytes_used, i * 2);
+    EXPECT_EQ(reporter.store[i].net_heap_growth, i * 4);
+    EXPECT_EQ(reporter.store[i].total_allocated_bytes, i * 10);
+  }
+
+  delete mm;
+}