From 1c64a36c5b8ee75d462b3fe7a9d020c66a2a1094 Mon Sep 17 00:00:00 2001
From: Mircea Trofin <mtrofin@google.com>
Date: Fri, 11 Aug 2023 04:46:36 -0700
Subject: [PATCH] [perf-counters] Fix pause/resume (#1643)

* [perf-counters] Fix pause/resume

Using `state.PauseTiming() / state.ResumeTiming()` was broken.

Thanks [@virajbshah] for the the repro testcase.

* ran clang-format over the whole perf_counters_test.cc

* Remove check that perf counters are 0 on `Pause`, since `Pause`/`Resume`
sequences would cause a non-0 counter value

* both upper and lower bound for the with/without resume counters

---------

Co-authored-by: dominic <510002+dmah42@users.noreply.github.com>
---
 src/benchmark.cc           |  3 +-
 test/perf_counters_test.cc | 67 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/src/benchmark.cc b/src/benchmark.cc
index 3e9c7f9642..a4fd2e92e9 100644
--- a/src/benchmark.cc
+++ b/src/benchmark.cc
@@ -229,8 +229,7 @@ void State::PauseTiming() {
     for (const auto& name_and_measurement : measurements) {
       auto name = name_and_measurement.first;
       auto measurement = name_and_measurement.second;
-      BM_CHECK_EQ(std::fpclassify(double{counters[name]}), FP_ZERO);
-      counters[name] = Counter(measurement, Counter::kAvgIterations);
+      counters[name] += Counter(measurement, Counter::kAvgIterations);
     }
   }
 }
diff --git a/test/perf_counters_test.cc b/test/perf_counters_test.cc
index 98cadda0b2..5419947fff 100644
--- a/test/perf_counters_test.cc
+++ b/test/perf_counters_test.cc
@@ -1,8 +1,8 @@
+#include <cstdarg>
 #undef NDEBUG
 
-#include "../src/perf_counters.h"
-
 #include "../src/commandlineflags.h"
+#include "../src/perf_counters.h"
 #include "benchmark/benchmark.h"
 #include "output_test.h"
 
@@ -21,17 +21,78 @@ static void BM_Simple(benchmark::State& state) {
 BENCHMARK(BM_Simple);
 ADD_CASES(TC_JSONOut, {{"\"name\": \"BM_Simple\",$"}});
 
+const int kIters = 1000000;
+
+void BM_WithoutPauseResume(benchmark::State& state) {
+  int n = 0;
+
+  for (auto _ : state) {
+    for (auto i = 0; i < kIters; ++i) {
+      n = 1 - n;
+      benchmark::DoNotOptimize(n);
+    }
+  }
+}
+
+BENCHMARK(BM_WithoutPauseResume);
+ADD_CASES(TC_JSONOut, {{"\"name\": \"BM_WithoutPauseResume\",$"}});
+
+void BM_WithPauseResume(benchmark::State& state) {
+  int m = 0, n = 0;
+
+  for (auto _ : state) {
+    for (auto i = 0; i < kIters; ++i) {
+      n = 1 - n;
+      benchmark::DoNotOptimize(n);
+    }
+
+    state.PauseTiming();
+    for (auto j = 0; j < kIters; ++j) {
+      m = 1 - m;
+      benchmark::DoNotOptimize(m);
+    }
+    state.ResumeTiming();
+  }
+}
+
+BENCHMARK(BM_WithPauseResume);
+
+ADD_CASES(TC_JSONOut, {{"\"name\": \"BM_WithPauseResume\",$"}});
+
 static void CheckSimple(Results const& e) {
   CHECK_COUNTER_VALUE(e, double, "CYCLES", GT, 0);
   CHECK_COUNTER_VALUE(e, double, "BRANCHES", GT, 0.0);
 }
+
+double withoutPauseResumeInstrCount = 0.0;
+double withPauseResumeInstrCount = 0.0;
+
+static void CheckInstrCount(double* counter, Results const& e) {
+  BM_CHECK_GT(e.NumIterations(), 0);
+  *counter = e.GetAs<double>("INSTRUCTIONS") / e.NumIterations();
+}
+
+static void CheckInstrCountWithoutResume(Results const& e) {
+  CheckInstrCount(&withoutPauseResumeInstrCount, e);
+}
+
+static void CheckInstrCountWithResume(Results const& e) {
+  CheckInstrCount(&withPauseResumeInstrCount, e);
+}
+
 CHECK_BENCHMARK_RESULTS("BM_Simple", &CheckSimple);
+CHECK_BENCHMARK_RESULTS("BM_WithoutPauseResume", &CheckInstrCountWithoutResume);
+CHECK_BENCHMARK_RESULTS("BM_WithPauseResume", &CheckInstrCountWithResume);
 
 int main(int argc, char* argv[]) {
   if (!benchmark::internal::PerfCounters::kSupported) {
     return 0;
   }
-  benchmark::FLAGS_benchmark_perf_counters = "CYCLES,BRANCHES";
+  benchmark::FLAGS_benchmark_perf_counters = "CYCLES,BRANCHES,INSTRUCTIONS";
   benchmark::internal::PerfCounters::Initialize();
   RunOutputTests(argc, argv);
+
+  BM_CHECK_GT(withPauseResumeInstrCount, kIters);
+  BM_CHECK_GT(withoutPauseResumeInstrCount, kIters);
+  BM_CHECK_LT(withPauseResumeInstrCount, 1.5 * withoutPauseResumeInstrCount);
 }