Support "shared" histograms between processes.

base/metrics/histogram.cc

Index: base/metrics/histogram.cc
diff --git a/base/metrics/histogram.cc b/base/metrics/histogram.cc
index 083856bf87618379a964eb2095aa6cae4d7039c1..d2a502b7475111e7cd8828ad2ba35528eafea748 100644
--- a/base/metrics/histogram.cc
+++ b/base/metrics/histogram.cc
@@ -18,7 +18,9 @@
 #include "base/debug/alias.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/metrics/histogram_persistence.h"
 #include "base/metrics/metrics_hashes.h"
+#include "base/metrics/persistent_memory_allocator.h"
 #include "base/metrics/sample_vector.h"
 #include "base/metrics/statistics_recorder.h"
 #include "base/pickle.h"
@@ -83,45 +85,138 @@ typedef HistogramBase::Sample Sample;
 // static
 const size_t Histogram::kBucketCount_MAX = 16384u;
 
-HistogramBase* Histogram::FactoryGet(const std::string& name,
-                                     Sample minimum,
-                                     Sample maximum,
-                                     size_t bucket_count,
-                                     int32 flags) {
-  bool valid_arguments =
-      InspectConstructionArguments(name, &minimum, &maximum, &bucket_count);
-  DCHECK(valid_arguments);
 
-  HistogramBase* histogram = StatisticsRecorder::FindHistogram(name);
+class Histogram::Factory {
+ public:
+  Factory(const std::string& name,
+          HistogramBase::Sample minimum,
+          HistogramBase::Sample maximum,
+          size_t bucket_count,
+          int32 flags)
+    : Factory(name, HISTOGRAM, minimum, maximum, bucket_count, flags) {}
+
+  HistogramBase* Build();
+
+ protected:
+  Factory(const std::string& name,
+          HistogramType histogram_type,
+          HistogramBase::Sample minimum,
+          HistogramBase::Sample maximum,
+          size_t bucket_count,
+          int32 flags)
+    : name_(name),
+      histogram_type_(histogram_type),
+      minimum_(minimum),
+      maximum_(maximum),
+      bucket_count_(bucket_count),
+      flags_(flags) {}
+
+  virtual BucketRanges* CreateRanges() {
+    BucketRanges* ranges = new BucketRanges(bucket_count_ + 1);
+    Histogram::InitializeBucketRanges(minimum_, maximum_, ranges);
+    return ranges;
+  }
+
+  virtual HistogramBase* HeapAlloc(const BucketRanges* ranges) {
+    return new Histogram(name_, minimum_, maximum_, ranges);
+  }
+
+  virtual void FillHistogram(HistogramBase* histogram) {}

[Alexei Svitkine (slow), 2015/12/21 19:32:10]

Please document these methods and the class.

[bcwhite, 2015/12/22 20:50:38]

Done.

+
+  const std::string& name_;
+  const HistogramType histogram_type_;
+  HistogramBase::Sample minimum_;
+  HistogramBase::Sample maximum_;
+  size_t bucket_count_;
+  int32 flags_;
+};

[Alexei Svitkine (slow), 2015/12/21 19:32:10]

Add a private: section and DISALLOW_COPY_AND_ASSIGN().

Move any members that are not referenced by subclasses to private section.

For the other ones, add a comment about why they're in protected section.

[bcwhite, 2015/12/22 20:50:37]

Done.

+
+HistogramBase* Histogram::Factory::Build() {
+  ImportPersistentHistograms();
+
+  HistogramBase* histogram = StatisticsRecorder::FindHistogram(name_);
   if (!histogram) {
     // To avoid racy destruction at shutdown, the following will be leaked.
-    BucketRanges* ranges = new BucketRanges(bucket_count + 1);
-    InitializeBucketRanges(minimum, maximum, ranges);
+    const BucketRanges* created_ranges = CreateRanges();
     const BucketRanges* registered_ranges =
-        StatisticsRecorder::RegisterOrDeleteDuplicateRanges(ranges);
+        StatisticsRecorder::RegisterOrDeleteDuplicateRanges(created_ranges);
+
+    // Sometimes the bucket info is dynamically determined in create_ranges().

[Alexei Svitkine (slow), 2015/12/21 19:32:10]

Nit: create_ranges() -> CreateRanges().

[bcwhite, 2015/12/22 20:50:37]

Done.

+    if (bucket_count_ == 0) {
+      bucket_count_ = registered_ranges->bucket_count();
+      minimum_ = registered_ranges->range(1);
+      maximum_ = registered_ranges->range(bucket_count_ - 1);
+    }
+
+    PersistentMemoryAllocator::Reference histogram_ref = 0;
+    HistogramBase* tentative_histogram = nullptr;
+    PersistentMemoryAllocator* allocator =
+        GetPersistentHistogramMemoryAllocator();
+    if (allocator) {
+      flags_ |= HistogramBase::kIsPersistent;
+      tentative_histogram = AllocatePersistentHistogram(
+          allocator,
+          histogram_type_,
+          name_,
+          minimum_,
+          maximum_,
+          registered_ranges,
+          flags_,
+          &histogram_ref);
+    }
+    if (!tentative_histogram) {

[Alexei Svitkine (slow), 2015/12/21 19:32:10]

Either make this an else or mention in what legit situation can the allocator
return null and why those situation should be handled gracefully. (i.e. if it's
not expected that it should fail, it should be a DCHECK for example)

[bcwhite, 2015/12/22 20:50:38]

Done.

+      flags_ &= ~HistogramBase::kIsPersistent;
+      tentative_histogram = HeapAlloc(registered_ranges);

[Alexei Svitkine (slow), 2015/12/21 19:32:10]

Should this also clear histogram_ref?

Or is the expectation that if the allocator returned null, the ref is also 0? If
so, there should be a DCHECK on that since you have code that checks
histogram_ref on line 179.

[bcwhite, 2015/12/22 20:50:37]

Done.

+    }
 
-    Histogram* tentative_histogram =
-        new Histogram(name, minimum, maximum, registered_ranges);
+    FillHistogram(tentative_histogram);
 
-    tentative_histogram->SetFlags(flags);
+    tentative_histogram->SetFlags(flags_);

[Alexei Svitkine (slow), 2015/12/21 19:32:10]

Nit: Move this about FillHistogram() and don't separate the two by a blank line.

[bcwhite, 2015/12/22 20:50:37]

That will require that any overriden FillHistogram() call the "super" version. 
That should probably be the case anyway.

Done.

     histogram =
         StatisticsRecorder::RegisterOrDeleteDuplicate(tentative_histogram);
+
+    // Persistent histograms need some follow-up processing.
+    if (histogram_ref) {
+      // If the created persistent histogram is canonical then it needs to be
+      // marked as "iterable" in order to be found by other processes.
+      if (histogram == tentative_histogram)
+        GetPersistentHistogramMemoryAllocator()->MakeIterable(histogram_ref);
+      // If it's not the canonical one then a race condition must have caused
+      // two to be created. The allocator does not support releasing the
+      // acquired memory so just change the type to be empty.
+      else
+        GetPersistentHistogramMemoryAllocator()->SetType(histogram_ref, 0);

[Alexei Svitkine (slow), 2015/12/21 19:32:11]

Can this logic be contained in the allocator? i.e. just have a single function
on it that takes the histogram ref and a bool about whether it was registered or
not.

[bcwhite, 2015/12/22 20:50:38]

Done.

+    }
   }
 
-  DCHECK_EQ(HISTOGRAM, histogram->GetHistogramType());
-  if (!histogram->HasConstructionArguments(minimum, maximum, bucket_count)) {
+  DCHECK_EQ(histogram_type_, histogram->GetHistogramType());
+  if (bucket_count_ != 0 &&
+      !histogram->HasConstructionArguments(minimum_, maximum_, bucket_count_)) {
     // The construction arguments do not match the existing histogram.  This can
     // come about if an extension updates in the middle of a chrome run and has
     // changed one of them, or simply by bad code within Chrome itself.  We
     // return NULL here with the expectation that bad code in Chrome will crash
     // on dereference, but extension/Pepper APIs will guard against NULL and not
     // crash.
-    DLOG(ERROR) << "Histogram " << name << " has bad construction arguments";
-    return NULL;
+    DLOG(ERROR) << "Histogram " << name_ << " has bad construction arguments";
+    return nullptr;
   }
   return histogram;
 }
 
+
+HistogramBase* Histogram::FactoryGet(const std::string& name,
+                                     Sample minimum,
+                                     Sample maximum,
+                                     size_t bucket_count,
+                                     int32 flags) {
+  bool valid_arguments =
+      InspectConstructionArguments(name, &minimum, &maximum, &bucket_count);
+  DCHECK(valid_arguments);
+
+  return Factory(name, minimum, maximum, bucket_count, flags).Build();
+}
+
 HistogramBase* Histogram::FactoryTimeGet(const std::string& name,
                                          TimeDelta minimum,
                                          TimeDelta maximum,
@@ -343,6 +438,22 @@ Histogram::Histogram(const std::string& name,
     samples_.reset(new SampleVector(HashMetricName(name), ranges));
 }
 
+Histogram::Histogram(const std::string& name,
+                     Sample minimum,
+                     Sample maximum,
+                     const BucketRanges* ranges,
+                     HistogramBase::AtomicCount* counts,
+                     size_t counts_size,
+                     HistogramSamples::Metadata* meta)
+  : HistogramBase(name),
+    bucket_ranges_(ranges),
+    declared_min_(minimum),
+    declared_max_(maximum) {
+  if (ranges)
+    samples_.reset(new SampleVector(HashMetricName(name),
+                                    counts, counts_size, meta, ranges));
+}
+
 Histogram::~Histogram() {
 }
 
@@ -544,7 +655,43 @@ void Histogram::GetCountAndBucketData(Count* count,
 // buckets.
 //------------------------------------------------------------------------------
 
-LinearHistogram::~LinearHistogram() {}
+class LinearHistogram::Factory : public Histogram::Factory {
+ public:
+  Factory(const std::string& name,
+          HistogramBase::Sample minimum,
+          HistogramBase::Sample maximum,
+          size_t bucket_count,
+          int32 flags,
+          const DescriptionPair* descriptions)
+    : Histogram::Factory(name, LINEAR_HISTOGRAM, minimum, maximum,
+                         bucket_count, flags) {
+    descriptions_ = descriptions;
+  }
+
+ protected:

[Alexei Svitkine (slow), 2015/12/21 19:32:10]

Nit: private:

and add DISALLOW_COPY_AND_ASSIGN()

[bcwhite, 2015/12/22 20:50:37]

Done.

+  BucketRanges* CreateRanges() override {
+    BucketRanges* ranges = new BucketRanges(bucket_count_ + 1);
+    LinearHistogram::InitializeBucketRanges(minimum_, maximum_, ranges);
+    return ranges;
+  }
+
+  HistogramBase* HeapAlloc(const BucketRanges* ranges) override {
+    return new LinearHistogram(name_, minimum_, maximum_, ranges);
+  }
+
+  void FillHistogram(HistogramBase* base_histogram) override {
+    LinearHistogram* histogram = static_cast<LinearHistogram*>(base_histogram);
+    // Set range descriptions.
+    if (descriptions_) {
+      for (int i = 0; descriptions_[i].description; ++i) {
+        histogram->bucket_description_[descriptions_[i].sample] =
+            descriptions_[i].description;
+      }
+    }
+  }
+
+  const DescriptionPair* descriptions_;
+};
 
 HistogramBase* LinearHistogram::FactoryGet(const std::string& name,
                                            Sample minimum,
@@ -593,42 +740,8 @@ HistogramBase* LinearHistogram::FactoryGetWithRangeDescription(
       name, &minimum, &maximum, &bucket_count);
   DCHECK(valid_arguments);
 
-  HistogramBase* histogram = StatisticsRecorder::FindHistogram(name);
-  if (!histogram) {
-    // To avoid racy destruction at shutdown, the following will be leaked.
-    BucketRanges* ranges = new BucketRanges(bucket_count + 1);
-    InitializeBucketRanges(minimum, maximum, ranges);
-    const BucketRanges* registered_ranges =
-        StatisticsRecorder::RegisterOrDeleteDuplicateRanges(ranges);
-
-    LinearHistogram* tentative_histogram =
-        new LinearHistogram(name, minimum, maximum, registered_ranges);
-
-    // Set range descriptions.
-    if (descriptions) {
-      for (int i = 0; descriptions[i].description; ++i) {
-        tentative_histogram->bucket_description_[descriptions[i].sample] =
-            descriptions[i].description;
-      }
-    }
-
-    tentative_histogram->SetFlags(flags);
-    histogram =
-        StatisticsRecorder::RegisterOrDeleteDuplicate(tentative_histogram);
-  }
-
-  DCHECK_EQ(LINEAR_HISTOGRAM, histogram->GetHistogramType());
-  if (!histogram->HasConstructionArguments(minimum, maximum, bucket_count)) {
-    // The construction arguments do not match the existing histogram.  This can
-    // come about if an extension updates in the middle of a chrome run and has
-    // changed one of them, or simply by bad code within Chrome itself.  We
-    // return NULL here with the expectation that bad code in Chrome will crash
-    // on dereference, but extension/Pepper APIs will guard against NULL and not
-    // crash.
-    DLOG(ERROR) << "Histogram " << name << " has bad construction arguments";
-    return NULL;
-  }
-  return histogram;
+  return Factory(name, minimum, maximum, bucket_count, flags, descriptions)
+      .Build();
 }
 
 HistogramType LinearHistogram::GetHistogramType() const {
@@ -642,6 +755,21 @@ LinearHistogram::LinearHistogram(const std::string& name,
     : Histogram(name, minimum, maximum, ranges) {
 }
 
+LinearHistogram::LinearHistogram(const std::string& name,
+                                 Sample minimum,
+                                 Sample maximum,
+                                 const BucketRanges* ranges,
+                                 HistogramBase::AtomicCount* counts,
+                                 size_t counts_size,
+                                 HistogramSamples::Metadata* meta)
+    : Histogram(name,
+                minimum,
+                maximum,
+                ranges,
+                counts,
+                counts_size,
+                meta) {}
+
 double LinearHistogram::GetBucketSize(Count current, size_t i) const {
   DCHECK_GT(ranges(i + 1), ranges(i));
   // Adjacent buckets with different widths would have "surprisingly" many (few)
@@ -705,26 +833,26 @@ HistogramBase* LinearHistogram::DeserializeInfoImpl(PickleIterator* iter) {
 // This section provides implementation for BooleanHistogram.
 //------------------------------------------------------------------------------
 
-HistogramBase* BooleanHistogram::FactoryGet(const std::string& name,
-                                            int32 flags) {
-  HistogramBase* histogram = StatisticsRecorder::FindHistogram(name);
-  if (!histogram) {
-    // To avoid racy destruction at shutdown, the following will be leaked.
-    BucketRanges* ranges = new BucketRanges(4);
-    LinearHistogram::InitializeBucketRanges(1, 2, ranges);
-    const BucketRanges* registered_ranges =
-        StatisticsRecorder::RegisterOrDeleteDuplicateRanges(ranges);
+class BooleanHistogram::Factory : public Histogram::Factory {
+ public:
+  Factory(const std::string& name, int32 flags)
+    : Histogram::Factory(name, BOOLEAN_HISTOGRAM, 1, 2, 3, flags) {}
 
-    BooleanHistogram* tentative_histogram =
-        new BooleanHistogram(name, registered_ranges);
+ protected:
+  BucketRanges* CreateRanges() {

[Alexei Svitkine (slow), 2015/12/21 19:32:10]

Can this class just extend LinearHistogram and avoid this override?

[bcwhite, 2015/12/22 20:50:37]

Yes but it would involve changing the LinearHistogram ctor to take a
histogram_type parameter and passing a null descriptions value.  This seemed
cleaner overall.

+    BucketRanges* ranges = new BucketRanges(3 + 1);
+    LinearHistogram::InitializeBucketRanges(1, 2, ranges);
+    return ranges;
+  }
 
-    tentative_histogram->SetFlags(flags);
-    histogram =
-        StatisticsRecorder::RegisterOrDeleteDuplicate(tentative_histogram);
+  HistogramBase* HeapAlloc(const BucketRanges* ranges) override {
+    return new BooleanHistogram(name_, ranges);
   }
+};
 
-  DCHECK_EQ(BOOLEAN_HISTOGRAM, histogram->GetHistogramType());
-  return histogram;
+HistogramBase* BooleanHistogram::FactoryGet(const std::string& name,
+                                            int32 flags) {
+  return Factory(name, flags).Build();
 }
 
 HistogramBase* BooleanHistogram::FactoryGet(const char* name, int32 flags) {
@@ -739,6 +867,12 @@ BooleanHistogram::BooleanHistogram(const std::string& name,
                                    const BucketRanges* ranges)
     : LinearHistogram(name, 1, 2, ranges) {}
 
+BooleanHistogram::BooleanHistogram(const std::string& name,
+                                   const BucketRanges* ranges,
+                                   HistogramBase::AtomicCount* counts,
+                                   HistogramSamples::Metadata* meta)
+    : LinearHistogram(name, 1, 2, ranges, counts, 2, meta) {}
+
 HistogramBase* BooleanHistogram::DeserializeInfoImpl(PickleIterator* iter) {
   std::string histogram_name;
   int flags;
@@ -765,30 +899,46 @@ HistogramBase* BooleanHistogram::DeserializeInfoImpl(PickleIterator* iter) {
 // CustomHistogram:
 //------------------------------------------------------------------------------
 
+class CustomHistogram::Factory : public Histogram::Factory {
+ public:
+  Factory(const std::string& name,
+          const std::vector<Sample>* custom_ranges,
+          int32 flags)
+    : Histogram::Factory(name, CUSTOM_HISTOGRAM, 0, 0, 0, flags) {
+    custom_ranges_ = custom_ranges;
+  }
+
+ protected:
+  BucketRanges* CreateRanges() {
+    // Remove the duplicates in the custom ranges array.
+    std::vector<int> ranges = *custom_ranges_;
+    ranges.push_back(0);  // Ensure we have a zero value.
+    ranges.push_back(HistogramBase::kSampleType_MAX);
+    std::sort(ranges.begin(), ranges.end());
+    ranges.erase(std::unique(ranges.begin(), ranges.end()), ranges.end());
+
+    BucketRanges* bucket_ranges = new BucketRanges(ranges.size());
+    for (size_t i = 0; i < ranges.size(); i++) {
+      bucket_ranges->set_range(i, ranges[i]);
+    }
+    bucket_ranges->ResetChecksum();
+    return bucket_ranges;
+  }
+
+  HistogramBase* HeapAlloc(const BucketRanges* ranges) override {
+    return new CustomHistogram(name_, ranges);
+  }
+
+  const std::vector<Sample>* custom_ranges_;
+};
+
 HistogramBase* CustomHistogram::FactoryGet(
     const std::string& name,
     const std::vector<Sample>& custom_ranges,
     int32 flags) {
   CHECK(ValidateCustomRanges(custom_ranges));
 
-  HistogramBase* histogram = StatisticsRecorder::FindHistogram(name);
-  if (!histogram) {
-    BucketRanges* ranges = CreateBucketRangesFromCustomRanges(custom_ranges);
-    const BucketRanges* registered_ranges =
-        StatisticsRecorder::RegisterOrDeleteDuplicateRanges(ranges);
-
-    // To avoid racy destruction at shutdown, the following will be leaked.
-    CustomHistogram* tentative_histogram =
-        new CustomHistogram(name, registered_ranges);
-
-    tentative_histogram->SetFlags(flags);
-
-    histogram =
-        StatisticsRecorder::RegisterOrDeleteDuplicate(tentative_histogram);
-  }
-
-  DCHECK_EQ(histogram->GetHistogramType(), CUSTOM_HISTOGRAM);
-  return histogram;
+  return Factory(name, &custom_ranges, flags).Build();
 }
 
 HistogramBase* CustomHistogram::FactoryGet(
@@ -824,6 +974,19 @@ CustomHistogram::CustomHistogram(const std::string& name,
                 ranges->range(ranges->bucket_count() - 1),
                 ranges) {}
 
+CustomHistogram::CustomHistogram(const std::string& name,
+                                 const BucketRanges* ranges,
+                                 HistogramBase::AtomicCount* counts,
+                                 size_t counts_size,
+                                 HistogramSamples::Metadata* meta)
+    : Histogram(name,
+                ranges->range(1),
+                ranges->range(ranges->bucket_count() - 1),
+                ranges,
+                counts,
+                counts_size,
+                meta) {}
+
 bool CustomHistogram::SerializeInfoImpl(Pickle* pickle) const {
   if (!Histogram::SerializeInfoImpl(pickle))
     return false;
@@ -886,22 +1049,4 @@ bool CustomHistogram::ValidateCustomRanges(
   return has_valid_range;
 }
 
-// static
-BucketRanges* CustomHistogram::CreateBucketRangesFromCustomRanges(
-      const std::vector<Sample>& custom_ranges) {
-  // Remove the duplicates in the custom ranges array.
-  std::vector<int> ranges = custom_ranges;
-  ranges.push_back(0);  // Ensure we have a zero value.
-  ranges.push_back(HistogramBase::kSampleType_MAX);
-  std::sort(ranges.begin(), ranges.end());
-  ranges.erase(std::unique(ranges.begin(), ranges.end()), ranges.end());
-
-  BucketRanges* bucket_ranges = new BucketRanges(ranges.size());
-  for (size_t i = 0; i < ranges.size(); i++) {
-    bucket_ranges->set_range(i, ranges[i]);
-  }
-  bucket_ranges->ResetChecksum();
-  return bucket_ranges;
-}
-
 }  // namespace base