Fix use of history timers in background threads.

src/counters.cc

Index: src/counters.cc
diff --git a/src/counters.cc b/src/counters.cc
index ff76aa650a8b6cfd3911ba390c3772a7c82ceaeb..5464789ca038b24cf9dfcbed9052ead1cbeb8aa9 100644
--- a/src/counters.cc
+++ b/src/counters.cc
@@ -77,27 +77,23 @@ void* Histogram::CreateHistogram() const {
   return counters_->CreateHistogram(name_, min_, max_, num_buckets_);
 }
 
-
-// Start the timer.
-void HistogramTimer::Start() {
-  if (Enabled()) {
-    timer_.Start();
-  }
-  Logger::CallEventLogger(counters()->isolate(), name(), Logger::START, true);
+void TimedHistogram::Start(base::ElapsedTimer* timer, Isolate* isolate) {
+  if (Enabled()) timer->Start();
+  if (isolate) Logger::CallEventLogger(isolate, name(), Logger::START, true);

[Mircea Trofin, 2017/06/22 18:42:23]

why would it not have an isolate?

also, for consistency, check isolate != nullptr

[kschimpf, 2017/06/22 20:38:31]

The only need for the isolate is for logging, and only that can be done with the
(main thread) isolate.

The previous definition of HistogramTimer always did logging. For thread safety,
we can't do this for collection of background threads.

 }
 
-
-// Stop the timer and record the results.
-void HistogramTimer::Stop() {
+void TimedHistogram::Stop(base::ElapsedTimer* timer, Isolate* isolate) {
   if (Enabled()) {
-    int64_t sample = resolution_ == MICROSECOND
-                         ? timer_.Elapsed().InMicroseconds()
-                         : timer_.Elapsed().InMilliseconds();
     // Compute the delta between start and stop, in microseconds.
+    int64_t sample = resolution_ == HistogramTimerResolution::MICROSECOND
+                         ? timer->Elapsed().InMicroseconds()
+                         : timer->Elapsed().InMilliseconds();
+    timer->Stop();
     AddSample(static_cast<int>(sample));
-    timer_.Stop();
   }
-  Logger::CallEventLogger(counters()->isolate(), name(), Logger::END, true);
+  if (isolate != nullptr) {
+    Logger::CallEventLogger(isolate, name(), Logger::END, true);
+  }
 }
 
 Counters::Counters(Isolate* isolate)
@@ -131,10 +127,10 @@ Counters::Counters(Isolate* isolate)
     HistogramTimer Counters::*member;
     const char* caption;
     int max;
-    HistogramTimer::Resolution res;
+    HistogramTimerResolution res;
   } kHistogramTimers[] = {
 #define HT(name, caption, max, res) \
-  {&Counters::name##_, #caption, max, HistogramTimer::res},
+  {&Counters::name##_, #caption, max, HistogramTimerResolution::res},
       HISTOGRAM_TIMER_LIST(HT)
 #undef HT
   };
@@ -143,6 +139,22 @@ Counters::Counters(Isolate* isolate)
         HistogramTimer(timer.caption, 0, timer.max, timer.res, 50, this);
   }
 
+  static const struct {
+    TimedHistogram Counters::*member;
+    const char* caption;
+    int max;
+    HistogramTimerResolution res;
+  } kTimedHistograms[] = {
+#define HT(name, caption, max, res) \
+  {&Counters::name##_, #caption, max, HistogramTimerResolution::res},
+      TIMED_HISTOGRAM_LIST(HT)
+#undef HT
+  };
+  for (const auto& timer : kTimedHistograms) {
+    this->*timer.member =
+        TimedHistogram(timer.caption, 0, timer.max, timer.res, 50, this);

[Mircea Trofin, 2017/06/22 18:42:23]

what's "50" - I realize it's done the same above, but could we add a constant to
document what it represents?

[kschimpf, 2017/06/22 20:38:31]

Its the number of buckets. The current macros do not specify a value. I'll make
a symbolic constant for it.

Note: I also fixed the 50 in line 168.

+  }
+
   static const struct {
     AggregatableHistogramTimer Counters::*member;
     const char* caption;
@@ -294,6 +306,10 @@ void Counters::ResetCreateHistogramFunction(CreateHistogramCallback f) {
     HISTOGRAM_TIMER_LIST(HT)
 #undef HT
 
+#define HT(name, caption, max, res) name##_.Reset();
+    TIMED_HISTOGRAM_LIST(HT)
+#undef HT
+
 #define AHT(name, caption) name##_.Reset();
     AGGREGATABLE_HISTOGRAM_TIMER_LIST(AHT)
 #undef AHT