From 2a9965bd0ecf90393d785425b97c5045fc0b5316 Mon Sep 17 00:00:00 2001 From: kschimpf Date: Mon, 29 May 2017 11:18:25 -0700 Subject: [PATCH] Move StatsTable into the Counters class. By moving StatsTable from class Isolate to class Counters, it make the class StatsTable thead safe. This is needed because these two classes call each other, and for background compilation, instances of the Counters class can persist longer that the corresponding Isolate it came from. It also removes unnecessary hops to the the Isolate, and checks if the StatsTable has been created, for these communications. BUG=v8:6361 Review-Url: https://codereview.chromium.org/2906063002 Cr-Commit-Position: refs/heads/master@{#45576} --- src/api.cc | 2 +- src/counters.cc | 45 +++++++++++++++++---------------- src/counters.h | 56 ++++++++++++++++++++++------------------- src/isolate.cc | 8 +++--- test/cctest/test-api.cc | 2 +- 5 files changed, 58 insertions(+), 55 deletions(-) diff --git a/src/api.cc b/src/api.cc index 102836d313..1eed09226a 100644 --- a/src/api.cc +++ b/src/api.cc @@ -8666,7 +8666,7 @@ void Isolate::SetUseCounterCallback(UseCounterCallback callback) { void Isolate::SetCounterFunction(CounterLookupCallback callback) { i::Isolate* isolate = reinterpret_cast(this); - isolate->stats_table()->SetCounterFunction(callback, isolate); + isolate->stats_table()->SetCounterFunction(callback); } diff --git a/src/counters.cc b/src/counters.cc index 327b203afb..c98078658f 100644 --- a/src/counters.cc +++ b/src/counters.cc @@ -15,23 +15,24 @@ namespace v8 { namespace internal { -StatsTable::StatsTable() - : lookup_function_(NULL), +StatsTable::StatsTable(Counters* counters) + : counters_(counters), + lookup_function_(NULL), create_histogram_function_(NULL), add_histogram_sample_function_(NULL) {} -void StatsTable::SetCounterFunction(CounterLookupCallback f, Isolate* isolate) { +void StatsTable::SetCounterFunction(CounterLookupCallback f) { lookup_function_ = f; - if (!isolate->InitializeCounters()) isolate->counters()->ResetCounters(); + counters_->ResetCounters(); } int* StatsCounterBase::FindLocationInStatsTable() const { - return isolate_->stats_table()->FindLocation(name_); + return counters_->stats_table()->FindLocation(name_); } -StatsCounterThreadSafe::StatsCounterThreadSafe(Isolate* isolate, +StatsCounterThreadSafe::StatsCounterThreadSafe(Counters* counters, const char* name) - : StatsCounterBase(isolate, name) { + : StatsCounterBase(counters, name) { GetPtr(); } @@ -78,13 +79,13 @@ int* StatsCounterThreadSafe::GetPtr() { void Histogram::AddSample(int sample) { if (Enabled()) { - isolate()->stats_table()->AddHistogramSample(histogram_, sample); + counters_->stats_table()->AddHistogramSample(histogram_, sample); } } void* Histogram::CreateHistogram() const { - return isolate()->stats_table()-> - CreateHistogram(name_, min_, max_, num_buckets_); + return counters_->stats_table()->CreateHistogram(name_, min_, max_, + num_buckets_); } @@ -93,7 +94,7 @@ void HistogramTimer::Start() { if (Enabled()) { timer_.Start(); } - Logger::CallEventLogger(isolate(), name(), Logger::START, true); + Logger::CallEventLogger(counters()->isolate(), name(), Logger::START, true); } @@ -107,13 +108,14 @@ void HistogramTimer::Stop() { AddSample(static_cast(sample)); timer_.Stop(); } - Logger::CallEventLogger(isolate(), name(), Logger::END, true); + Logger::CallEventLogger(counters()->isolate(), name(), Logger::END, true); } Counters::Counters(Isolate* isolate) - : + : isolate_(isolate), + stats_table_(this), // clang format off -#define SC(name, caption) name##_(isolate, "c:" #caption), +#define SC(name, caption) name##_(this, "c:" #caption), STATS_COUNTER_TS_LIST(SC) #undef SC // clang format on @@ -133,7 +135,7 @@ Counters::Counters(Isolate* isolate) for (const auto& histogram : kHistograms) { this->*histogram.member = Histogram(histogram.caption, histogram.min, histogram.max, - histogram.num_buckets, isolate); + histogram.num_buckets, this); } static const struct { @@ -149,7 +151,7 @@ Counters::Counters(Isolate* isolate) }; for (const auto& timer : kHistogramTimers) { this->*timer.member = - HistogramTimer(timer.caption, 0, timer.max, timer.res, 50, isolate); + HistogramTimer(timer.caption, 0, timer.max, timer.res, 50, this); } static const struct { @@ -162,7 +164,7 @@ Counters::Counters(Isolate* isolate) }; for (const auto& aht : kAggregatableHistogramTimers) { this->*aht.member = - AggregatableHistogramTimer(aht.caption, 0, 10000000, 50, isolate); + AggregatableHistogramTimer(aht.caption, 0, 10000000, 50, this); } static const struct { @@ -174,8 +176,7 @@ Counters::Counters(Isolate* isolate) #undef HP }; for (const auto& percentage : kHistogramPercentages) { - this->*percentage.member = - Histogram(percentage.caption, 0, 101, 100, isolate); + this->*percentage.member = Histogram(percentage.caption, 0, 101, 100, this); } // Exponential histogram assigns bucket limits to points @@ -194,7 +195,7 @@ Counters::Counters(Isolate* isolate) }; for (const auto& histogram : kLegacyMemoryHistograms) { this->*histogram.member = - Histogram(histogram.caption, 1000, 500000, 50, isolate); + Histogram(histogram.caption, 1000, 500000, 50, this); } // For n = 100, low = 4000, high = 2000000: the factor = 1.06. @@ -210,7 +211,7 @@ Counters::Counters(Isolate* isolate) }; for (const auto& histogram : kMemoryHistograms) { this->*histogram.member = - Histogram(histogram.caption, 4000, 2000000, 100, isolate); + Histogram(histogram.caption, 4000, 2000000, 100, this); this->*histogram.aggregated = AggregatedMemoryHistogram(&(this->*histogram.member)); } @@ -252,7 +253,7 @@ Counters::Counters(Isolate* isolate) }; // clang-format on for (const auto& counter : kStatsCounters) { - this->*counter.member = StatsCounter(isolate, counter.caption); + this->*counter.member = StatsCounter(this, counter.caption); } } diff --git a/src/counters.h b/src/counters.h index 37367561c9..fc7395e033 100644 --- a/src/counters.h +++ b/src/counters.h @@ -25,12 +25,14 @@ namespace internal { // counters for monitoring. Counters can be looked up and // manipulated by name. +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. - void SetCounterFunction(CounterLookupCallback f, Isolate* isolate); + void SetCounterFunction(CounterLookupCallback f); // Register an application-defined function to create // a histogram for passing to the AddHistogramSample function @@ -80,29 +82,29 @@ class StatsTable { } private: - StatsTable(); + explicit StatsTable(Counters* counters); + Counters* counters_; CounterLookupCallback lookup_function_; CreateHistogramCallback create_histogram_function_; AddHistogramSampleCallback add_histogram_sample_function_; - friend class Isolate; + friend class Counters; DISALLOW_COPY_AND_ASSIGN(StatsTable); }; // Base class for stats counters. class StatsCounterBase { - public: - StatsCounterBase() {} - StatsCounterBase(Isolate* isolate, const char* name) - : isolate_(isolate), name_(name), ptr_(nullptr) {} - protected: - Isolate* isolate_; + Counters* counters_; const char* name_; int* ptr_; + StatsCounterBase() {} + StatsCounterBase(Counters* counters, const char* name) + : counters_(counters), name_(name), ptr_(nullptr) {} + void SetLoc(int* loc, int value) { *loc = value; } void IncrementLoc(int* loc) { (*loc)++; } void IncrementLoc(int* loc, int value) { (*loc) += value; } @@ -123,8 +125,8 @@ class StatsCounterBase { class StatsCounter : public StatsCounterBase { public: StatsCounter() { } - StatsCounter(Isolate* isolate, const char* name) - : StatsCounterBase(isolate, name), lookup_done_(false) {} + StatsCounter(Counters* counters, const char* name) + : StatsCounterBase(counters, name), lookup_done_(false) {} // Sets the counter to a specific value. void Set(int value) { @@ -185,7 +187,7 @@ class StatsCounter : public StatsCounterBase { // (i.e. not workers). class StatsCounterThreadSafe : public StatsCounterBase { public: - StatsCounterThreadSafe(Isolate* isolate, const char* name); + StatsCounterThreadSafe(Counters* counters, const char* name); void Set(int Value); void Increment(); @@ -213,18 +215,15 @@ class StatsCounterThreadSafe : public StatsCounterBase { class Histogram { public: Histogram() { } - Histogram(const char* name, - int min, - int max, - int num_buckets, - Isolate* isolate) + 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), - isolate_(isolate) { } + counters_(counters) {} // Add a single sample to this histogram. void AddSample(int sample); @@ -251,7 +250,7 @@ class Histogram { return histogram_; } - Isolate* isolate() const { return isolate_; } + Counters* counters() const { return counters_; } private: void* CreateHistogram() const; @@ -262,7 +261,7 @@ class Histogram { int num_buckets_; void* histogram_; bool lookup_done_; - Isolate* isolate_; + Counters* counters_; }; // A HistogramTimer allows distributions of results to be created. @@ -275,8 +274,8 @@ class HistogramTimer : public Histogram { HistogramTimer() {} HistogramTimer(const char* name, int min, int max, Resolution resolution, - int num_buckets, Isolate* isolate) - : Histogram(name, min, max, num_buckets, isolate), + int num_buckets, Counters* counters) + : Histogram(name, min, max, num_buckets, counters), resolution_(resolution) {} // Start the timer. @@ -358,8 +357,8 @@ class AggregatableHistogramTimer : public Histogram { public: AggregatableHistogramTimer() {} AggregatableHistogramTimer(const char* name, int min, int max, - int num_buckets, Isolate* isolate) - : Histogram(name, min, max, num_buckets, isolate) {} + int num_buckets, Counters* counters) + : Histogram(name, min, max, num_buckets, counters) {} // Start/stop the "outer" scope. void Start() { time_ = base::TimeDelta(); } @@ -1335,7 +1334,14 @@ class Counters : public std::enable_shared_from_this { RuntimeCallStats* runtime_call_stats() { return &runtime_call_stats_; } + StatsTable* stats_table() { return &stats_table_; } + + Isolate* isolate() { return isolate_; } + private: + Isolate* isolate_; + StatsTable stats_table_; + #define HR(name, caption, min, max, num_buckets) Histogram name##_; HISTOGRAM_RANGE_LIST(HR) #undef HR @@ -1401,8 +1407,6 @@ class Counters : public std::enable_shared_from_this { RuntimeCallStats runtime_call_stats_; - friend class Isolate; - DISALLOW_IMPLICIT_CONSTRUCTORS(Counters); }; diff --git a/src/isolate.cc b/src/isolate.cc index 3f8096b847..8ab12603e3 100644 --- a/src/isolate.cc +++ b/src/isolate.cc @@ -2540,7 +2540,6 @@ Isolate::~Isolate() { store_stub_cache_ = NULL; delete code_aging_helper_; code_aging_helper_ = NULL; - delete stats_table_; stats_table_ = NULL; delete materialized_object_store_; @@ -2844,10 +2843,9 @@ bool Isolate::Init(Deserializer* des) { // Initialized lazily to allow early // v8::V8::SetAddHistogramSampleFunction calls. StatsTable* Isolate::stats_table() { - if (stats_table_ == NULL) { - stats_table_ = new StatsTable; - } - return stats_table_; + if (stats_table_ != nullptr) return stats_table_; + InitializeCounters(); + return stats_table_ = counters_->stats_table(); } diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index 959506ddc2..39412258bc 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -23937,7 +23937,7 @@ TEST(EventLogging) { isolate->SetEventLogger(StoringEventLoggerCallback); v8::internal::HistogramTimer histogramTimer( "V8.Test", 0, 10000, v8::internal::HistogramTimer::MILLISECOND, 50, - reinterpret_cast(isolate)); + reinterpret_cast(isolate)->counters()); histogramTimer.Start(); CHECK_EQ(0, strcmp("V8.Test", last_event_message)); CHECK_EQ(0, last_event_status);