Create a thread safe version of StatsCounters and use.

src/counters.h

Index: src/counters.h
diff --git a/src/counters.h b/src/counters.h
index bb917cc518eded7f61e99b3edb58db2bf7f68f93..4a8391beb9a4eccf6b1472332f846030f791be6c 100644
--- a/src/counters.h
+++ b/src/counters.h
@@ -28,10 +28,9 @@ namespace internal {
 class StatsTable {
  public:
   // Register an application-defined function where
-  // counters can be looked up.
-  void SetCounterFunction(CounterLookupCallback f) {
-    lookup_function_ = f;
-  }
+  // counters can be looked up. Note: Must be called on main thread,
+  // so that threaded stats counters can be created now.
+  void SetCounterFunction(CounterLookupCallback f, Isolate* isolate);
 
   // Register an application-defined function to create
   // a histogram for passing to the AddHistogramSample function
@@ -107,33 +106,17 @@ class StatsCounter {
       : isolate_(isolate), name_(name), ptr_(NULL), lookup_done_(false) { }
 
   // Sets the counter to a specific value.
-  void Set(int value) {
-    int* loc = GetPtr();
-    if (loc) *loc = value;
-  }
+  void Set(int value) { Set(GetPtr(), value); }

[jochen (gone - plz use gerrit), 2017/05/23 11:29:20]

having that all public looks odd. Could the thread safe version inherit from
StatsCounter? Maybe there should be a StatsCounterBase or something that both
inherit from?

[kschimpf, 2017/05/23 16:53:07]

I don't understand this comment. The thread safe version inherits from this
class (StatsCounter). I did not widen the public API. The public non-pointer
version calls a protected pointer version.

However, I also see the argument for possibly creating a base class. Changing to
use a common base class.

 
   // Increments the counter.
-  void Increment() {
-    int* loc = GetPtr();
-    if (loc) (*loc)++;
-  }
+  void Increment() { Increment(GetPtr()); }
 
-  void Increment(int value) {
-    int* loc = GetPtr();
-    if (loc)
-      (*loc) += value;
-  }
+  void Increment(int value) { Increment(GetPtr(), value); }
 
   // Decrements the counter.
-  void Decrement() {
-    int* loc = GetPtr();
-    if (loc) (*loc)--;
-  }
+  void Decrement() { Decrement(GetPtr()); }
 
-  void Decrement(int value) {
-    int* loc = GetPtr();
-    if (loc) (*loc) -= value;
-  }
+  void Decrement(int value) { Decrement(GetPtr(), value); }
 
   // Is this counter enabled?
   // Returns false if table is full.
@@ -162,15 +145,63 @@ class StatsCounter {
     return ptr_;
   }
 
- private:
   int* FindLocationInStatsTable() const;
 
+  void Set(int* loc, int value) {
+    if (loc) *loc = value;
+  }
+
+  void Increment(int* loc) {
+    if (loc) (*loc)++;
+  }
+
+  void Increment(int* loc, int value) {
+    if (loc) (*loc) += value;
+  }
+
+  void Decrement(int* loc) {
+    if (loc) (*loc)--;
+  }
+
+  void Decrement(int* loc, int value) {
+    if (loc) (*loc) -= value;
+  }
+
   Isolate* isolate_;
   const char* name_;
   int* ptr_;
   bool lookup_done_;
 };
 
+// Thread safe version of StatsCounter. WARNING: Unlike StatsCounter,
+// StatsCounterThreadSafe's constructor and method Reset() actually do
+// the table lookup, and should be called from the main thread
+// (i.e. not workers).
+class StatsCounterThreadSafe : public StatsCounter {
+ public:
+  explicit StatsCounterThreadSafe(Isolate* isolate, const char* name);

[jochen (gone - plz use gerrit), 2017/05/23 11:29:21]

nit. no explicit

[kschimpf, 2017/05/23 16:53:07]

Done.

+
+  void Set(int Value);
+  void Increment();
+  void Increment(int value);
+  void Decrement();
+  void Decrement(int value);
+  bool Enabled() { return ptr_ != NULL; }
+  int* GetInternalPointer() {
+    DCHECK(ptr_ != NULL);
+    return ptr_;
+  }
+  void Reset();
+
+ private:
+  int* GetPtr();
+
+  base::Mutex mutex_;
+
+ private:
+  DISALLOW_IMPLICIT_CONSTRUCTORS(StatsCounterThreadSafe);
+};
+
 // A Histogram represents a dynamically created histogram in the StatsTable.
 // It will be registered with the histogram system on first use.
 class Histogram {
@@ -1167,14 +1198,18 @@ class RuntimeCallTimerScope {
   /* Total code size (including metadata) of baseline code or bytecode. */     \
   SC(total_baseline_code_size, V8.TotalBaselineCodeSize)                       \
   /* Total count of functions compiled using the baseline compiler. */         \
-  SC(total_baseline_compile_count, V8.TotalBaselineCompileCount)               \
-  SC(wasm_generated_code_size, V8.WasmGeneratedCodeBytes)                      \
-  SC(wasm_reloc_size, V8.WasmRelocBytes)                                       \
+  SC(total_baseline_compile_count, V8.TotalBaselineCompileCount)
+
+#define STATS_COUNTER_TS_LIST(SC)                         \
+  SC(wasm_generated_code_size, V8.WasmGeneratedCodeBytes) \
+  SC(wasm_reloc_size, V8.WasmRelocBytes)                  \
   SC(wasm_lazily_compiled_functions, V8.WasmLazilyCompiledFunctions)
 
 // This file contains all the v8 counters that are in use.
-class Counters {
+class Counters : public std::enable_shared_from_this<Counters> {
  public:
+  explicit Counters(Isolate* isolate);
+
 #define HR(name, caption, min, max, num_buckets) \
   Histogram* name() { return &name##_; }
   HISTOGRAM_RANGE_LIST(HR)
@@ -1214,6 +1249,11 @@ class Counters {
   STATS_COUNTER_LIST_2(SC)
 #undef SC
 
+#define SC(name, caption) \
+  StatsCounterThreadSafe* name() { return &name##_; }
+  STATS_COUNTER_TS_LIST(SC)
+#undef SC
+
 #define SC(name) \
   StatsCounter* count_of_##name() { return &count_of_##name##_; } \
   StatsCounter* size_of_##name() { return &size_of_##name##_; }
@@ -1244,6 +1284,7 @@ class Counters {
   CODE_AGE_LIST_COMPLETE(SC)
 #undef SC
 
+  // clang-format off

[jochen (gone - plz use gerrit), 2017/05/23 11:29:20]

hum, why?

[kschimpf, 2017/05/23 16:53:07]

Because clang-format changes the following Defines/definitions into a highly
nested unreadable mess. I do not know why this is (whether it is a change in
clang-format, or this code was originally committed without clang formatting).

   enum Id {
 #define RATE_ID(name, caption, max, res) k_##name,
     HISTOGRAM_TIMER_LIST(RATE_ID)
@@ -1261,6 +1302,7 @@ class Counters {
 #define COUNTER_ID(name, caption) k_##name,
     STATS_COUNTER_LIST_1(COUNTER_ID)
     STATS_COUNTER_LIST_2(COUNTER_ID)
+    STATS_COUNTER_TS_LIST(COUNTER_ID)
 #undef COUNTER_ID
 #define COUNTER_ID(name) kCountOf##name, kSizeOf##name,
     INSTANCE_TYPE_LIST(COUNTER_ID)
@@ -1279,6 +1321,7 @@ class Counters {
 #undef COUNTER_ID
     stats_counter_count
   };
+  // clang-format on
 
   void ResetCounters();
   void ResetHistograms();
@@ -1322,6 +1365,10 @@ class Counters {
   STATS_COUNTER_LIST_2(SC)
 #undef SC
 
+#define SC(name, caption) StatsCounterThreadSafe name##_;
+  STATS_COUNTER_TS_LIST(SC)
+#undef SC
+
 #define SC(name) \
   StatsCounter size_of_##name##_; \
   StatsCounter count_of_##name##_;
@@ -1350,8 +1397,6 @@ class Counters {
 
   friend class Isolate;
 
-  explicit Counters(Isolate* isolate);
-
   DISALLOW_IMPLICIT_CONSTRUCTORS(Counters);
 };