Create a thread safe version of StatsCounters and use.

src/counters.h

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef V8_COUNTERS_H_
 #define V8_COUNTERS_H_
 
 #include "include/v8.h"
 #include "src/allocation.h"
 #include "src/base/atomic-utils.h"
@@ skipping 10 matching lines @@
 namespace v8 {
 namespace internal {
 
 // StatsCounters is an interface for plugging into external
 // counters for monitoring.  Counters can be looked up and
 // manipulated by name.
 
 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
   void SetCreateHistogramFunction(CreateHistogramCallback f) {
     create_histogram_function_ = f;
   }
 
   // Register an application-defined function to add a sample
   // to a histogram created with CreateHistogram function
   void SetAddHistogramSampleFunction(AddHistogramSampleCallback f) {
@@ skipping 55 matching lines @@
 // The row has a 32bit value for each process/thread in the table and also
 // a name (stored in the table metadata).  Since the storage location can be
 // thread-specific, this class cannot be shared across threads.
 class StatsCounter {
  public:
   StatsCounter() { }
   explicit StatsCounter(Isolate* isolate, const char* name)
       : 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.
   bool Enabled() {
     return GetPtr() != NULL;
   }
 
   // Get the internal pointer to the counter. This is used
   // by the code generator to emit code that manipulates a
   // given counter without calling the runtime system.
   int* GetInternalPointer() {
     int* loc = GetPtr();
     DCHECK(loc != NULL);
     return loc;
   }
 
   // Reset the cached internal pointer.
   void Reset() { lookup_done_ = false; }
 
  protected:
   // Returns the cached address of this counter location.
   int* GetPtr() {
     if (lookup_done_) return ptr_;
     lookup_done_ = true;
     ptr_ = FindLocationInStatsTable();
     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 {
  public:
   Histogram() { }
   Histogram(const char* name,
             int min,
             int max,
             int num_buckets,
             Isolate* isolate)
@@ skipping 976 matching lines @@
   SC(code_space_bytes_used, V8.MemoryCodeSpaceBytesUsed)                       \
   SC(map_space_bytes_available, V8.MemoryMapSpaceBytesAvailable)               \
   SC(map_space_bytes_committed, V8.MemoryMapSpaceBytesCommitted)               \
   SC(map_space_bytes_used, V8.MemoryMapSpaceBytesUsed)                         \
   SC(lo_space_bytes_available, V8.MemoryLoSpaceBytesAvailable)                 \
   SC(lo_space_bytes_committed, V8.MemoryLoSpaceBytesCommitted)                 \
   SC(lo_space_bytes_used, V8.MemoryLoSpaceBytesUsed)                           \
   /* 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)
 #undef HR
 
 #define HT(name, caption, max, res) \
   HistogramTimer* name() { return &name##_; }
   HISTOGRAM_TIMER_LIST(HT)
 #undef HT
 
@@ skipping 19 matching lines @@
   }
   HISTOGRAM_MEMORY_LIST(HM)
 #undef HM
 
 #define SC(name, caption) \
   StatsCounter* name() { return &name##_; }
   STATS_COUNTER_LIST_1(SC)
   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##_; }
   INSTANCE_TYPE_LIST(SC)
 #undef SC
 
 #define SC(name) \
   StatsCounter* count_of_CODE_TYPE_##name() \
     { return &count_of_CODE_TYPE_##name##_; } \
   StatsCounter* size_of_CODE_TYPE_##name() \
@@ skipping 10 matching lines @@
 #undef SC
 
 #define SC(name) \
   StatsCounter* count_of_CODE_AGE_##name() \
     { return &count_of_CODE_AGE_##name##_; } \
   StatsCounter* size_of_CODE_AGE_##name() \
     { return &size_of_CODE_AGE_##name##_; }
   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)
 #undef RATE_ID
 #define AGGREGATABLE_ID(name, caption) k_##name,
     AGGREGATABLE_HISTOGRAM_TIMER_LIST(AGGREGATABLE_ID)
 #undef AGGREGATABLE_ID
 #define PERCENTAGE_ID(name, caption) k_##name,
     HISTOGRAM_PERCENTAGE_LIST(PERCENTAGE_ID)
 #undef PERCENTAGE_ID
 #define MEMORY_ID(name, caption) k_##name,
     HISTOGRAM_LEGACY_MEMORY_LIST(MEMORY_ID)
     HISTOGRAM_MEMORY_LIST(MEMORY_ID)
 #undef MEMORY_ID
 #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)
 #undef COUNTER_ID
 #define COUNTER_ID(name) kCountOfCODE_TYPE_##name, \
     kSizeOfCODE_TYPE_##name,
     CODE_KIND_LIST(COUNTER_ID)
 #undef COUNTER_ID
 #define COUNTER_ID(name) kCountOfFIXED_ARRAY__##name, \
     kSizeOfFIXED_ARRAY__##name,
     FIXED_ARRAY_SUB_INSTANCE_TYPE_LIST(COUNTER_ID)
 #undef COUNTER_ID
 #define COUNTER_ID(name) kCountOfCODE_AGE__##name, \
     kSizeOfCODE_AGE__##name,
     CODE_AGE_LIST_COMPLETE(COUNTER_ID)
 #undef COUNTER_ID
     stats_counter_count
   };
+  // clang-format on
 
   void ResetCounters();
   void ResetHistograms();
   void InitializeHistograms();
 
   RuntimeCallStats* runtime_call_stats() { return &runtime_call_stats_; }
 
  private:
 #define HR(name, caption, min, max, num_buckets) Histogram name##_;
   HISTOGRAM_RANGE_LIST(HR)
@@ skipping 23 matching lines @@
   AggregatedMemoryHistogram<Histogram> aggregated_##name##_;
   HISTOGRAM_MEMORY_LIST(HM)
 #undef HM
 
 #define SC(name, caption) \
   StatsCounter name##_;
   STATS_COUNTER_LIST_1(SC)
   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##_;
   INSTANCE_TYPE_LIST(SC)
 #undef SC
 
 #define SC(name) \
   StatsCounter size_of_CODE_TYPE_##name##_; \
   StatsCounter count_of_CODE_TYPE_##name##_;
   CODE_KIND_LIST(SC)
 #undef SC
 
 #define SC(name) \
   StatsCounter size_of_FIXED_ARRAY_##name##_; \
   StatsCounter count_of_FIXED_ARRAY_##name##_;
   FIXED_ARRAY_SUB_INSTANCE_TYPE_LIST(SC)
 #undef SC
 
 #define SC(name) \
   StatsCounter size_of_CODE_AGE_##name##_; \
   StatsCounter count_of_CODE_AGE_##name##_;
   CODE_AGE_LIST_COMPLETE(SC)
 #undef SC
 
   RuntimeCallStats runtime_call_stats_;
 
   friend class Isolate;
 
-  explicit Counters(Isolate* isolate);
-
   DISALLOW_IMPLICIT_CONSTRUCTORS(Counters);
 };
 
 }  // namespace internal
 }  // namespace v8
 
 #endif  // V8_COUNTERS_H_