De-duplicate ranges information in persistent memory.

By having BucketRanges keep a reference to where the data can be found
in persistent memory, the existing de-dup of the BucketRanges object
also serves to avoid duplicating the data in persistent memory.

Simple testing shows allocated persistent histogram memory in the
browser is reduced by about 22%.  This should be similar in all
processes though somewhat less as the number of histograms held goes
down.

BUG=705342

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

base/metrics/bucket_ranges.h

@@ -24,6 +24,7 @@
 
 #include <limits.h>
 
+#include "base/atomicops.h"
 #include "base/base_export.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_base.h"
@@ -58,6 +59,17 @@ class BASE_EXPORT BucketRanges {
   // Return true iff |other| object has same ranges_ as |this| object's ranges_.
   bool Equals(const BucketRanges* other) const;
 
+  // Set and get a reference into persistent memory where this bucket data
+  // can be found (and re-used). These calls are internally atomic with no
+  // safety against overwriting an existing value since though it is wasteful
+  // to have multiple identical persistent records, it is still safe.
+  void set_persistent_reference(uint32_t ref) const {
+    subtle::NoBarrier_Store(&persistent_reference_, ref);
+  }
+  uint32_t persistent_reference() const {
+    return subtle::NoBarrier_Load(&persistent_reference_);
+  }
+
  private:
   // A monotonically increasing list of values which determine which bucket to
   // put a sample into.  For each index, show the smallest sample that can be
@@ -71,6 +83,12 @@ class BASE_EXPORT BucketRanges {
   // noise on UMA dashboard.
   uint32_t checksum_;
 
+  // A reference into a global PersistentMemoryAllocator where the ranges
+  // information is stored. This allows for the record to be created once and
+  // re-used simply by having all histograms with the same ranges use the
+  // same reference.
+  mutable subtle::Atomic32 persistent_reference_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(BucketRanges);
 };
 

base/metrics/persistent_histogram_allocator.cc

@@ -340,24 +340,49 @@ std::unique_ptr<HistogramBase> PersistentHistogramAllocator::AllocateHistogram(
       return nullptr;
     }
 
-    size_t ranges_count = bucket_count + 1;
-    size_t ranges_bytes = ranges_count * sizeof(HistogramBase::Sample);
+    // Since the StasticsRecorder keeps a global collection of BucketRanges
+    // objects for re-use, it would be dangerous for one to hold a reference
+    // from a persistent allocator that is not the global one (which is
+    // permanent once set). If this stops being the case, this check can
+    // become an "if" condition beside "!ranges_ref" below and before
+    // set_persistent_reference() farther down.
+    DCHECK_EQ(this, GlobalHistogramAllocator::Get());
+
+    // Re-use an existing BucketRanges persistent allocation if one is known;
+    // otherwise, create one.
+    PersistentMemoryAllocator::Reference ranges_ref =
+        bucket_ranges->persistent_reference();
+    if (!ranges_ref) {
+      size_t ranges_count = bucket_count + 1;
+      size_t ranges_bytes = ranges_count * sizeof(HistogramBase::Sample);
+      ranges_ref =
+          memory_allocator_->Allocate(ranges_bytes, kTypeIdRangesArray);
+      if (ranges_ref) {
+        HistogramBase::Sample* ranges_data =
+            memory_allocator_->GetAsArray<HistogramBase::Sample>(
+                ranges_ref, kTypeIdRangesArray, ranges_count);
+        if (ranges_data) {
+          for (size_t i = 0; i < bucket_ranges->size(); ++i)
+            ranges_data[i] = bucket_ranges->range(i);
+          bucket_ranges->set_persistent_reference(ranges_ref);
+        } else {
+          // This should never happen but be tolerant if it does.
+          NOTREACHED();
+          ranges_ref = PersistentMemoryAllocator::kReferenceNull;
+        }
+      }
+    } else {
+      DCHECK_EQ(kTypeIdRangesArray, memory_allocator_->GetType(ranges_ref));
+    }
+
     PersistentMemoryAllocator::Reference counts_ref =
         memory_allocator_->Allocate(counts_bytes, kTypeIdCountsArray);
-    PersistentMemoryAllocator::Reference ranges_ref =
-        memory_allocator_->Allocate(ranges_bytes, kTypeIdRangesArray);
-    HistogramBase::Sample* ranges_data =
-        memory_allocator_->GetAsArray<HistogramBase::Sample>(
-            ranges_ref, kTypeIdRangesArray, ranges_count);
 
     // Only continue here if all allocations were successful. If they weren't,
     // there is no way to free the space but that's not really a problem since
     // the allocations only fail because the space is full or corrupt and so
     // any future attempts will also fail.
-    if (counts_ref && ranges_data && histogram_data) {
-      for (size_t i = 0; i < bucket_ranges->size(); ++i)
-        ranges_data[i] = bucket_ranges->range(i);
-
+    if (counts_ref && ranges_ref && histogram_data) {
       histogram_data->minimum = minimum;
       histogram_data->maximum = maximum;
       // |bucket_count| must fit within 32-bits or the allocation of the counts

base/metrics/persistent_histogram_allocator_unittest.cc

@@ -281,4 +281,41 @@ TEST_F(PersistentHistogramAllocatorTest, StatisticsRecorderMergeTest) {
   EXPECT_EQ(1, snapshot->GetCount(7));
 }
 
+TEST_F(PersistentHistogramAllocatorTest, RangesDeDuplication) {
+  // This corresponds to the "ranges_ref" field of the PersistentHistogramData
+  // structure defined (privately) inside persistent_histogram_allocator.cc.
+  const int kRangesRefIndex = 5;
+
+  // Create two histograms with the same ranges.
+  HistogramBase* histogram1 =
+      Histogram::FactoryGet("TestHistogram1", 1, 1000, 10, 0);
+  HistogramBase* histogram2 =
+      Histogram::FactoryGet("TestHistogram2", 1, 1000, 10, 0);
+  const uint32_t ranges_ref = static_cast<Histogram*>(histogram1)
+                                  ->bucket_ranges()
+                                  ->persistent_reference();
+  ASSERT_NE(0U, ranges_ref);
+  EXPECT_EQ(ranges_ref, static_cast<Histogram*>(histogram2)
+                            ->bucket_ranges()
+                            ->persistent_reference());
+
+  // Make sure that the persistent data record is also correct. Two histograms
+  // will be fetched; other allocations are not "iterable".
+  PersistentMemoryAllocator::Iterator iter(allocator_);
+  uint32_t type;
+  uint32_t ref1 = iter.GetNext(&type);
+  uint32_t ref2 = iter.GetNext(&type);
+  EXPECT_EQ(0U, iter.GetNext(&type));
+  EXPECT_NE(0U, ref1);
+  EXPECT_NE(0U, ref2);
+  EXPECT_NE(ref1, ref2);
+
+  uint32_t* data1 =
+      allocator_->GetAsArray<uint32_t>(ref1, 0, kRangesRefIndex + 1);
+  uint32_t* data2 =
+      allocator_->GetAsArray<uint32_t>(ref2, 0, kRangesRefIndex + 1);
+  EXPECT_EQ(ranges_ref, data1[kRangesRefIndex]);
+  EXPECT_EQ(ranges_ref, data2[kRangesRefIndex]);
+}
+
 }  // namespace base

base/metrics/statistics_recorder.cc

@@ -431,8 +431,24 @@ size_t StatisticsRecorder::GetHistogramCount() {
 
 // static
 void StatisticsRecorder::ForgetHistogramForTesting(base::StringPiece name) {
-  if (histograms_)
-    histograms_->erase(name);
+  if (!histograms_)
+    return;
+
+  HistogramMap::iterator found = histograms_->find(name);
+  if (found == histograms_->end())
+    return;
+
+  HistogramBase* base = found->second;
+  if (base->GetHistogramType() != SPARSE_HISTOGRAM) {
+    // When forgetting a histogram, it's likely that other information is
+    // also becoming invalid. Clear the persistent reference that may no
+    // longer be valid. There's no danger in this as, at worst, duplicates
+    // will be created in persistent memory.
+    Histogram* histogram = static_cast<Histogram*>(base);
+    histogram->bucket_ranges()->set_persistent_reference(0);
+  }
+
+  histograms_->erase(found);
 }
 
 // static