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..324e5545a67b5726f271cfc71e5416d90324a84c 100644
--- a/src/counters.cc
+++ b/src/counters.cc
@@ -77,27 +77,30 @@ void* Histogram::CreateHistogram() const {
   return counters_->CreateHistogram(name_, min_, max_, num_buckets_);
 }
 
+void TimedHistogram::Start(base::ElapsedTimer* timer, Isolate* isolate) {
+  if (Enabled()) timer->Start();
+  if (isolate) Logger::CallEventLogger(isolate, name(), Logger::START, true);
+}
 
-// Start the timer.
-void HistogramTimer::Start() {
+void TimedHistogram::Stop(base::ElapsedTimer* timer, Isolate* isolate) {
   if (Enabled()) {

[Camillo Bruni, 2017/06/12 08:14:03]

Given that this might be slightly performance critical I'd suggest putting the
ENABLED / FLAG_log_internal_timer_events / isolate check TimedHistogramScope in
counters-inl.h and then call Stop/Start that does the actual work.

I've done the same thing fort the RuntimeCallTimerScope and the actual Counter.

WDYT? (we could also postpone this to a follow-up CL)

[kschimpf, 2017/06/12 16:47:04]

This was previously in HistogramTimer as non-inlined code, so how are we adding
more overhead? Is there proof that we should be paying the code size penalty to
get performance?

On a side note, I do agree that the HistogramTimer::Start()/Stop() should be
inlined (to not add two levels of call).

[Camillo Bruni, 2017/06/22 07:26:13]

SGTM, let's see how this behaves in general first.

-    timer_.Start();
+    // 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));
   }
-  Logger::CallEventLogger(counters()->isolate(), name(), Logger::START, true);
+  if (isolate != nullptr)

[Camillo Bruni, 2017/06/12 08:14:03]

nit: style violation, please add braces.

[kschimpf, 2017/06/12 16:47:04]

Done.

+    Logger::CallEventLogger(isolate, name(), Logger::END, true);
 }
 
+void HistogramTimer::Start() {
+  TimedHistogram::Start(&timer_, counters()->isolate());
+}
 
-// Stop the timer and record the results.
 void HistogramTimer::Stop() {
-  if (Enabled()) {
-    int64_t sample = resolution_ == MICROSECOND
-                         ? timer_.Elapsed().InMicroseconds()
-                         : timer_.Elapsed().InMilliseconds();
-    // Compute the delta between start and stop, in microseconds.
-    AddSample(static_cast<int>(sample));
-    timer_.Stop();
-  }
-  Logger::CallEventLogger(counters()->isolate(), name(), Logger::END, true);
+  TimedHistogram::Stop(&timer_, counters()->isolate());
 }
 
 Counters::Counters(Isolate* isolate)
@@ -131,10 +134,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 +146,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);
+  }
+
   static const struct {
     AggregatableHistogramTimer Counters::*member;
     const char* caption;
@@ -294,6 +313,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