Localize counter class member functions.

This CL takes advantage of the fact that StatsCounter is now local to
the Counters class. This includes:

1) Method StatsTable::SetCreateHistogramFunction() was only called in
one spot (in api.cc), which also called Counters::ResetHistograms()
and Counters::InitializeHistorgram(). InitializeHistogram can be
folded into Histogram.Reset().

2) Since Histogram::Reset() now regenerats the histogram, we no longer
need the field lookup_done_. Therefore there is no longer a race
between updating ptr_ and lookup_done_, making the Histogram class
thread safe.

3) Made the constructors of several classes private (except for class
Counters), minimizing the scope that they are used. When the couldn't
be moved, add comment that they were public only for test cases.

4) Removed the need for a mutex lock on StatsCounter::Reset(), since
it is now guaranteed to only be called when
StatsTable::SetCounterFunction() is called.

BUG=v8:6361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng

Review-Url: https://codereview.chromium.org/2918703002
Cr-Commit-Position: refs/heads/master@{#45791}

src/api.cc

@@ -8730,23 +8730,20 @@ void Isolate::SetUseCounterCallback(UseCounterCallback callback) {
 
 void Isolate::SetCounterFunction(CounterLookupCallback callback) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(this);
-  isolate->stats_table()->SetCounterFunction(callback);
+  isolate->counters()->ResetCounterFunction(callback);
 }
 
 
 void Isolate::SetCreateHistogramFunction(CreateHistogramCallback callback) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(this);
-  isolate->stats_table()->SetCreateHistogramFunction(callback);
-  isolate->InitializeLoggingAndCounters();
-  isolate->counters()->ResetHistograms();
-  isolate->counters()->InitializeHistograms();
+  isolate->counters()->ResetCreateHistogramFunction(callback);
 }
 
 
 void Isolate::SetAddHistogramSampleFunction(
     AddHistogramSampleCallback callback) {
   reinterpret_cast<i::Isolate*>(this)
-      ->stats_table()
+      ->counters()
       ->SetAddHistogramSampleFunction(callback);
 }
 

src/counters.cc

@@ -16,25 +16,21 @@ namespace v8 {
 namespace internal {
 
 StatsTable::StatsTable(Counters* counters)
-    : counters_(counters),
-      lookup_function_(NULL),
+    : lookup_function_(NULL),
       create_histogram_function_(NULL),
       add_histogram_sample_function_(NULL) {}
 
 void StatsTable::SetCounterFunction(CounterLookupCallback f) {
   lookup_function_ = f;
-  counters_->ResetCounters();
 }
 
 int* StatsCounterBase::FindLocationInStatsTable() const {
-  return counters_->stats_table()->FindLocation(name_);
+  return counters_->FindLocation(name_);
 }
 
 StatsCounterThreadSafe::StatsCounterThreadSafe(Counters* counters,
                                                const char* name)
-    : StatsCounterBase(counters, name) {
-  GetPtr();
-}
+    : StatsCounterBase(counters, name) {}
 
 void StatsCounterThreadSafe::Set(int Value) {
   if (ptr_) {
@@ -71,21 +67,14 @@ void StatsCounterThreadSafe::Decrement(int value) {
   }
 }
 
-int* StatsCounterThreadSafe::GetPtr() {
-  base::LockGuard<base::Mutex> Guard(&mutex_);
-  ptr_ = FindLocationInStatsTable();
-  return ptr_;
-}
-
 void Histogram::AddSample(int sample) {
   if (Enabled()) {
-    counters_->stats_table()->AddHistogramSample(histogram_, sample);
+    counters_->AddHistogramSample(histogram_, sample);
   }
 }
 
 void* Histogram::CreateHistogram() const {
-  return counters_->stats_table()->CreateHistogram(name_, min_, max_,
-                                                   num_buckets_);
+  return counters_->CreateHistogram(name_, min_, max_, num_buckets_);
 }
 
 
@@ -257,7 +246,9 @@ Counters::Counters(Isolate* isolate)
   }
 }
 
-void Counters::ResetCounters() {
+void Counters::ResetCounterFunction(CounterLookupCallback f) {
+  stats_table_.SetCounterFunction(f);
+
 #define SC(name, caption) name##_.Reset();
   STATS_COUNTER_LIST_1(SC)
   STATS_COUNTER_LIST_2(SC)
@@ -292,8 +283,9 @@ void Counters::ResetCounters() {
 #undef SC
 }
 
+void Counters::ResetCreateHistogramFunction(CreateHistogramCallback f) {
+  stats_table_.SetCreateHistogramFunction(f);
 
-void Counters::ResetHistograms() {
 #define HR(name, caption, min, max, num_buckets) name##_.Reset();
   HISTOGRAM_RANGE_LIST(HR)
 #undef HR
@@ -316,29 +308,6 @@ void Counters::ResetHistograms() {
 #undef HM
 }
 
-void Counters::InitializeHistograms() {
-#define HR(name, caption, min, max, num_buckets) name##_.Enabled();
-  HISTOGRAM_RANGE_LIST(HR)
-#undef HR
-
-#define HT(name, caption, max, res) name##_.Enabled();
-  HISTOGRAM_TIMER_LIST(HT)
-#undef HT
-
-#define AHT(name, caption) name##_.Enabled();
-  AGGREGATABLE_HISTOGRAM_TIMER_LIST(AHT)
-#undef AHT
-
-#define HP(name, caption) name##_.Enabled();
-  HISTOGRAM_PERCENTAGE_LIST(HP)
-#undef HP
-
-#define HM(name, caption) name##_.Enabled();
-  HISTOGRAM_LEGACY_MEMORY_LIST(HM)
-  HISTOGRAM_MEMORY_LIST(HM)
-#undef HM
-}
-
 class RuntimeCallStatEntries {
  public:
   void Print(std::ostream& os) {

src/counters.h

@@ -29,19 +29,18 @@ class Counters;
 
 class StatsTable {
  public:
-  // Register an application-defined function where
-  // counters can be looked up. Note: Must be called on main thread,
-  // so that threaded stats counters can be created now.
+  // Register an application-defined function for recording
+  // subsequent counter statistics.
   void SetCounterFunction(CounterLookupCallback f);
 
-  // Register an application-defined function to create
-  // a histogram for passing to the AddHistogramSample function
+  // Register an application-defined function to create histograms for
+  // recording subsequent histogram samples.
   void SetCreateHistogramFunction(CreateHistogramCallback f) {
     create_histogram_function_ = f;
   }
 
   // Register an application-defined function to add a sample
-  // to a histogram created with CreateHistogram function
+  // to a histogram created with CreateHistogram function.
   void SetAddHistogramSampleFunction(AddHistogramSampleCallback f) {
     add_histogram_sample_function_ = f;
   }
@@ -82,15 +81,14 @@ class StatsTable {
   }
 
  private:
+  friend class Counters;
+
   explicit StatsTable(Counters* counters);
 
-  Counters* counters_;
   CounterLookupCallback lookup_function_;
   CreateHistogramCallback create_histogram_function_;
   AddHistogramSampleCallback add_histogram_sample_function_;
 
-  friend class Counters;
-
   DISALLOW_COPY_AND_ASSIGN(StatsTable);
 };
 
@@ -121,13 +119,10 @@ class StatsCounterBase {
 // Internally, a counter represents a value in a row of a StatsTable.
 // 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.
+// thread-specific, this class cannot be shared across threads. Note: This
+// class is not thread safe.
 class StatsCounter : public StatsCounterBase {
  public:
-  StatsCounter() { }
-  StatsCounter(Counters* counters, const char* name)
-      : StatsCounterBase(counters, name), lookup_done_(false) {}
-
   // Sets the counter to a specific value.
   void Set(int value) {
     if (int* loc = GetPtr()) SetLoc(loc, value);
@@ -166,10 +161,16 @@ class StatsCounter : public StatsCounterBase {
     return loc;
   }
 
+ private:
+  friend class Counters;
+
+  StatsCounter() {}
+  StatsCounter(Counters* counters, const char* name)
+      : StatsCounterBase(counters, name), lookup_done_(false) {}
+
   // Reset the cached internal pointer.
   void Reset() { lookup_done_ = false; }
 
- private:
   // Returns the cached address of this counter location.
   int* GetPtr() {
     if (lookup_done_) return ptr_;
@@ -181,14 +182,9 @@ class StatsCounter : public StatsCounterBase {
   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).
+// Thread safe version of StatsCounter.
 class StatsCounterThreadSafe : public StatsCounterBase {
  public:
-  StatsCounterThreadSafe(Counters* counters, const char* name);
-
   void Set(int Value);
   void Increment();
   void Increment(int value);
@@ -199,60 +195,49 @@ class StatsCounterThreadSafe : public StatsCounterBase {
     DCHECK(ptr_ != NULL);
     return ptr_;
   }
-  void Reset() { GetPtr(); }
 
  private:
-  int* GetPtr();
+  friend class Counters;
+
+  StatsCounterThreadSafe(Counters* counters, const char* name);
+  void Reset() { ptr_ = FindLocationInStatsTable(); }
 
   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.
+// A Histogram represents a dynamically created histogram in the
+// StatsTable.  Note: This class is thread safe.
 class Histogram {
  public:
-  Histogram() { }
-  Histogram(const char* name, int min, int max, int num_buckets,
-            Counters* counters)
-      : name_(name),
-        min_(min),
-        max_(max),
-        num_buckets_(num_buckets),
-        histogram_(NULL),
-        lookup_done_(false),
-        counters_(counters) {}
-
   // Add a single sample to this histogram.
   void AddSample(int sample);
 
   // Returns true if this histogram is enabled.
-  bool Enabled() {
-    return GetHistogram() != NULL;
-  }
-
-  // Reset the cached internal pointer.
-  void Reset() {
-    lookup_done_ = false;
-  }
+  bool Enabled() { return histogram_ != nullptr; }
 
   const char* name() { return name_; }
 
  protected:
-  // Returns the handle to the histogram.
-  void* GetHistogram() {
-    if (!lookup_done_) {
-      lookup_done_ = true;
-      histogram_ = CreateHistogram();
-    }
-    return histogram_;
-  }
+  Histogram() {}
+  Histogram(const char* name, int min, int max, int num_buckets,
+            Counters* counters)
+      : name_(name),
+        min_(min),
+        max_(max),
+        num_buckets_(num_buckets),
+        histogram_(nullptr),
+        counters_(counters) {}
 
   Counters* counters() const { return counters_; }
 
+  // Reset the cached internal pointer.
+  void Reset() { histogram_ = CreateHistogram(); }
+
  private:
+  friend class Counters;
+
   void* CreateHistogram() const;
 
   const char* name_;
@@ -260,7 +245,6 @@ class Histogram {
   int max_;
   int num_buckets_;
   void* histogram_;
-  bool lookup_done_;
   Counters* counters_;
 };
 
@@ -272,7 +256,7 @@ class HistogramTimer : public Histogram {
     MICROSECOND
   };
 
-  HistogramTimer() {}
+  // Note: public for testing purposes only.
   HistogramTimer(const char* name, int min, int max, Resolution resolution,
                  int num_buckets, Counters* counters)
       : Histogram(name, min, max, num_buckets, counters),
@@ -295,8 +279,12 @@ class HistogramTimer : public Histogram {
 #endif
 
  private:
+  friend class Counters;
+
   base::ElapsedTimer timer_;
   Resolution resolution_;
+
+  HistogramTimer() {}
 };
 
 // Helper class for scoping a HistogramTimer.
@@ -355,11 +343,6 @@ class HistogramTimerScope BASE_EMBEDDED {
 //     events to be timed.
 class AggregatableHistogramTimer : public Histogram {
  public:
-  AggregatableHistogramTimer() {}
-  AggregatableHistogramTimer(const char* name, int min, int max,
-                             int num_buckets, Counters* counters)
-      : Histogram(name, min, max, num_buckets, counters) {}
-
   // Start/stop the "outer" scope.
   void Start() { time_ = base::TimeDelta(); }
   void Stop() { AddSample(static_cast<int>(time_.InMicroseconds())); }
@@ -368,6 +351,13 @@ class AggregatableHistogramTimer : public Histogram {
   void Add(base::TimeDelta other) { time_ += other; }
 
  private:
+  friend class Counters;
+
+  AggregatableHistogramTimer() {}
+  AggregatableHistogramTimer(const char* name, int min, int max,
+                             int num_buckets, Counters* counters)
+      : Histogram(name, min, max, num_buckets, counters) {}
+
   base::TimeDelta time_;
 };
 
@@ -414,14 +404,7 @@ class AggregatedHistogramTimerScope {
 template <typename Histogram>
 class AggregatedMemoryHistogram {
  public:
-  AggregatedMemoryHistogram()
-      : is_initialized_(false),
-        start_ms_(0.0),
-        last_ms_(0.0),
-        aggregate_value_(0.0),
-        last_value_(0.0),
-        backing_histogram_(NULL) {}
-
+  // Note: public for testing purposes only.
   explicit AggregatedMemoryHistogram(Histogram* backing_histogram)
       : AggregatedMemoryHistogram() {
     backing_histogram_ = backing_histogram;
@@ -439,7 +422,17 @@ class AggregatedMemoryHistogram {
   void AddSample(double current_ms, double current_value);
 
  private:
+  friend class Counters;
+
+  AggregatedMemoryHistogram()
+      : is_initialized_(false),
+        start_ms_(0.0),
+        last_ms_(0.0),
+        aggregate_value_(0.0),
+        last_value_(0.0),
+        backing_histogram_(NULL) {}
   double Aggregate(double current_ms, double current_value);
+
   bool is_initialized_;
   double start_ms_;
   double last_ms_;
@@ -536,14 +529,14 @@ class RuntimeCallCounter final {
   void Add(base::TimeDelta delta) { time_ += delta.InMicroseconds(); }
 
  private:
+  friend class RuntimeCallStats;
+
   RuntimeCallCounter() {}
 
   const char* name_;
   int64_t count_;
   // Stored as int64_t so that its initialization can be deferred.
   int64_t time_;
-
-  friend class RuntimeCallStats;
 };
 
 // RuntimeCallTimer is used to keep track of the stack of currently active
@@ -1217,6 +1210,23 @@ class Counters : public std::enable_shared_from_this<Counters> {
  public:
   explicit Counters(Isolate* isolate);
 
+  // Register an application-defined function for recording
+  // subsequent counter statistics. Note: Must be called on the main
+  // thread.
+  void ResetCounterFunction(CounterLookupCallback f);
+
+  // Register an application-defined function to create histograms for
+  // recording subsequent histogram samples. Note: Must be called on
+  // the main thread.
+  void ResetCreateHistogramFunction(CreateHistogramCallback f);
+
+  // Register an application-defined function to add a sample
+  // to a histogram. Will be used in all subsequent sample additions.
+  // Note: Must be called on the main thread.
+  void SetAddHistogramSampleFunction(AddHistogramSampleCallback f) {
+    stats_table_.SetAddHistogramSampleFunction(f);
+  }
+
 #define HR(name, caption, min, max, num_buckets) \
   Histogram* name() { return &name##_; }
   HISTOGRAM_RANGE_LIST(HR)
@@ -1330,20 +1340,31 @@ class Counters : public std::enable_shared_from_this<Counters> {
   };
   // clang-format on
 
-  void ResetCounters();
-  void ResetHistograms();
-  void InitializeHistograms();
-
   RuntimeCallStats* runtime_call_stats() { return &runtime_call_stats_; }
 
-  StatsTable* stats_table() { return &stats_table_; }
-
-  Isolate* isolate() { return isolate_; }
-
  private:
+  friend class StatsTable;
+  friend class StatsCounterBase;
+  friend class Histogram;
+  friend class HistogramTimer;
+
   Isolate* isolate_;
   StatsTable stats_table_;
 
+  int* FindLocation(const char* name) {
+    return stats_table_.FindLocation(name);
+  }
+
+  void* CreateHistogram(const char* name, int min, int max, size_t buckets) {
+    return stats_table_.CreateHistogram(name, min, max, buckets);
+  }
+
+  void AddHistogramSample(void* histogram, int sample) {
+    stats_table_.AddHistogramSample(histogram, sample);
+  }
+
+  Isolate* isolate() { return isolate_; }
+
 #define HR(name, caption, min, max, num_buckets) Histogram name##_;
   HISTOGRAM_RANGE_LIST(HR)
 #undef HR

src/isolate.cc

@@ -2290,7 +2290,6 @@ Isolate::Isolate(bool enable_serializer)
       runtime_profiler_(NULL),
       compilation_cache_(NULL),
       logger_(NULL),
-      stats_table_(NULL),
       load_stub_cache_(NULL),
       store_stub_cache_(NULL),
       code_aging_helper_(NULL),
@@ -2544,7 +2543,6 @@ Isolate::~Isolate() {
   store_stub_cache_ = NULL;
   delete code_aging_helper_;
   code_aging_helper_ = NULL;
-  stats_table_ = NULL;
 
   delete materialized_object_store_;
   materialized_object_store_ = NULL;
@@ -2852,16 +2850,6 @@ bool Isolate::Init(Deserializer* des) {
 }
 
 
-// Initialized lazily to allow early
-// v8::V8::SetAddHistogramSampleFunction calls.
-StatsTable* Isolate::stats_table() {
-  if (stats_table_ != nullptr) return stats_table_;
-  InitializeCounters();
-  stats_table_ = counters_shared_->stats_table();
-  return stats_table_;
-}
-
-
 void Isolate::Enter() {
   Isolate* current_isolate = NULL;
   PerIsolateThreadData* current_data = CurrentPerIsolateThreadData();

src/isolate.h

@@ -883,7 +883,6 @@ class Isolate {
   }
   StackGuard* stack_guard() { return &stack_guard_; }
   Heap* heap() { return &heap_; }
-  StatsTable* stats_table();
   StubCache* load_stub_cache() { return load_stub_cache_; }
   StubCache* store_stub_cache() { return store_stub_cache_; }
   CodeAgingHelper* code_aging_helper() { return code_aging_helper_; }
@@ -1436,7 +1435,6 @@ class Isolate {
   base::RecursiveMutex break_access_;
   Logger* logger_;
   StackGuard stack_guard_;
-  StatsTable* stats_table_;
   StubCache* load_stub_cache_;
   StubCache* store_stub_cache_;
   CodeAgingHelper* code_aging_helper_;

test/unittests/counters-unittest.cc

@@ -28,9 +28,7 @@ class MockHistogram : public Histogram {
 
 class AggregatedMemoryHistogramTest : public ::testing::Test {
  public:
-  AggregatedMemoryHistogramTest() {
-    aggregated_ = AggregatedMemoryHistogram<MockHistogram>(&mock_);
-  }
+  AggregatedMemoryHistogramTest() : aggregated_(&mock_) {}
   virtual ~AggregatedMemoryHistogramTest() {}
 
   void AddSample(double current_ms, double current_value) {