fix memory manager result bug (#1941)

* fix memory manager result bug

* change is_valid to memory_iterations

* fix test

* some fixes

* fix test

...for msvc

* fix test

* fix test

add the correct explicitly casts

* fix msvc failure

* some fixes

* remove unnecessary include

Author: EfesX

include/benchmark/benchmark.h

@@ -306,6 +306,8 @@ namespace benchmark {
 class BenchmarkReporter;
 class State;
 
+using IterationCount = int64_t;
+
 // Define alias of Setup/Teardown callback function type
 using callback_function = std::function<void(const benchmark::State&)>;
 
@@ -387,14 +389,15 @@ BENCHMARK_EXPORT void SetDefaultTimeUnit(TimeUnit unit);
 // benchmark.
 class MemoryManager {
  public:
-  static const int64_t TombstoneValue;
+  static constexpr int64_t TombstoneValue = std::numeric_limits<int64_t>::max();
 
   struct Result {
     Result()
         : num_allocs(0),
           max_bytes_used(0),
           total_allocated_bytes(TombstoneValue),
-          net_heap_growth(TombstoneValue) {}
+          net_heap_growth(TombstoneValue),
+          memory_iterations(0) {}
 
     // The number of allocations made in total between Start and Stop.
     int64_t num_allocs;
@@ -410,6 +413,8 @@ class MemoryManager {
     // ie., total_allocated_bytes - total_deallocated_bytes.
     // Init'ed to TombstoneValue if metric not available.
     int64_t net_heap_growth;
+
+    IterationCount memory_iterations;
   };
 
   virtual ~MemoryManager() {}
@@ -659,8 +664,6 @@ enum BigO { oNone, o1, oN, oNSquared, oNCubed, oLogN, oNLogN, oAuto, oLambda };
 
 typedef int64_t ComplexityN;
 
-typedef int64_t IterationCount;
-
 enum StatisticUnit { kTime, kPercentage };
 
 // BigOFunc is passed to a benchmark in order to specify the asymptotic
@@ -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,10 @@ BenchmarkReporter::Run CreateRunReport(
     report.counters = results.counters;
 
     if (memory_iterations > 0) {
-      assert(memory_result != nullptr);
       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 +425,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 +437,9 @@ MemoryManager::Result* BenchmarkRunner::RunMemoryManager(
   manager->WaitForAllThreads();
   manager.reset();
   b.Teardown();
-  memory_manager->Stop(*memory_result);
+  MemoryManager::Result memory_result;
+  memory_manager->Stop(memory_result);
+  memory_result.memory_iterations = memory_iterations;
   return memory_result;
 }
 
@@ -508,7 +504,7 @@ 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

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.memory_iterations > 0) {
+    const auto& 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);
@@ -330,7 +330,4 @@ void JSONReporter::PrintRunData(Run const& run) {
   out << '\n';
 }
 
-const int64_t MemoryManager::TombstoneValue =
-    std::numeric_limits<int64_t>::max();
-
 }  // end namespace benchmark

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,101 @@
+#include <vector>
+
+#include "benchmark/benchmark.h"
+#include "gtest/gtest.h"
+
+namespace {
+
+using benchmark::ClearRegisteredBenchmarks;
+using benchmark::ConsoleReporter;
+using benchmark::MemoryManager;
+using benchmark::RegisterBenchmark;
+using benchmark::RunSpecifiedBenchmarks;
+using benchmark::State;
+using benchmark::internal::Benchmark;
+
+constexpr int N_REPETITIONS = 100;
+constexpr int N_ITERATIONS = 1;
+
+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;
+}
+class TestMemoryManager : public MemoryManager {
+  void Start() override {}
+  void Stop(Result& result) override {
+    result.num_allocs = num_allocs;
+    result.net_heap_growth = net_heap_growth;
+    result.max_bytes_used = max_bytes_used;
+    result.total_allocated_bytes = total_allocated_bytes;
+
+    num_allocs += 1;
+    max_bytes_used += 2;
+    net_heap_growth += 4;
+    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.memory_iterations) 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);
+    reset();
+  }
+  void TearDown() override { ClearRegisteredBenchmarks(); }
+};
+
+TEST_F(MemoryResultsTest, NoMMTest) {
+  RunSpecifiedBenchmarks(&reporter);
+  EXPECT_EQ(reporter.store.size(), 0);
+}
+
+TEST_F(MemoryResultsTest, ResultsTest) {
+  auto mm = std::make_unique<TestMemoryManager>();
+  RegisterMemoryManager(mm.get());
+
+  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, static_cast<int64_t>(i));
+    EXPECT_EQ(reporter.store[i].max_bytes_used, static_cast<int64_t>(i) * 2);
+    EXPECT_EQ(reporter.store[i].net_heap_growth, static_cast<int64_t>(i) * 4);
+    EXPECT_EQ(reporter.store[i].total_allocated_bytes,
+              static_cast<int64_t>(i) * 10);
+  }
+}
+
+}  // namespace