[histogram] Make histograms more resistant to overflows.

Instead of calculating deltas from cumulative snapshots, store unlogged
histogram samples in a separate HistogramSample object.
This will reduce overflows from long-running sessions given that data
between UMA pings fits into 32-bit integer.

BUG=719446

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

base/metrics/histogram.cc

@@ -414,7 +414,7 @@ bool Histogram::InspectConstructionArguments(const std::string& name,
 }
 
 uint64_t Histogram::name_hash() const {
-  return samples_->id();
+  return unlogged_samples_->id();
 }
 
 HistogramType Histogram::GetHistogramType() const {
@@ -445,50 +445,49 @@ void Histogram::AddCount(int value, int count) {
     NOTREACHED();
     return;
   }
-  samples_->Accumulate(value, count);
+  unlogged_samples_->Accumulate(value, count);
 
   FindAndRunCallback(value);
 }
 
 std::unique_ptr<HistogramSamples> Histogram::SnapshotSamples() const {
-  return SnapshotSampleVector();
+  return SnapshotAllSamples();
 }
 
 std::unique_ptr<HistogramSamples> Histogram::SnapshotDelta() {
   DCHECK(!final_delta_created_);
-
-  std::unique_ptr<HistogramSamples> snapshot = SnapshotSampleVector();
-  if (!logged_samples_) {
-    // If nothing has been previously logged, save this one as
-    // |logged_samples_| and gather another snapshot to return.
-    logged_samples_.swap(snapshot);
-    return SnapshotSampleVector();
-  }
-
-  // Subtract what was previously logged and update that information.
-  snapshot->Subtract(*logged_samples_);
+  // The code below has subtle thread-safety guarantees! All changes to
+  // the underlying SampleVectors use atomic integer operations, which guarantee
+  // eventual consistency, but do not guarantee full synchronization between
+  // different entries in the SampleVector. In particular, this means that
+  // concurrent updates to the histogram might result in the reported sum not
+  // matching the individual bucket counts; or there being some buckets that are
+  // logically updated "together", but end up being only partially updated when
+  // a snapshot is captured. Note that this is why it's important to subtract
+  // exactly the snapshotted unlogged samples, rather than simply resetting the
+  // vector: this way, the next snapshot will include any concurrent updates
+  // missed by the current snapshot.
+
+  std::unique_ptr<HistogramSamples> snapshot = SnapshotUnloggedSamples();
+  unlogged_samples_->Subtract(*snapshot);
   logged_samples_->Add(*snapshot);
+
   return snapshot;
 }
 
 std::unique_ptr<HistogramSamples> Histogram::SnapshotFinalDelta() const {
   DCHECK(!final_delta_created_);
   final_delta_created_ = true;
 
-  std::unique_ptr<HistogramSamples> snapshot = SnapshotSampleVector();
-
-  // Subtract what was previously logged and then return.
-  if (logged_samples_)
-    snapshot->Subtract(*logged_samples_);
-  return snapshot;
+  return SnapshotUnloggedSamples();
 }
 
 void Histogram::AddSamples(const HistogramSamples& samples) {
-  samples_->Add(samples);
+  unlogged_samples_->Add(samples);
 }
 
 bool Histogram::AddSamplesFromPickle(PickleIterator* iter) {
-  return samples_->AddFromPickle(iter);
+  return unlogged_samples_->AddFromPickle(iter);
 }
 
 // The following methods provide a graphical histogram display.
@@ -521,8 +520,10 @@ Histogram::Histogram(const std::string& name,
     bucket_ranges_(ranges),
     declared_min_(minimum),
     declared_max_(maximum) {
-  if (ranges)
-    samples_.reset(new SampleVector(HashMetricName(name), ranges));
+  if (ranges) {
+    unlogged_samples_.reset(new SampleVector(HashMetricName(name), ranges));
+    logged_samples_.reset(new SampleVector(unlogged_samples_->id(), ranges));
+  }
 }
 
 Histogram::Histogram(const std::string& name,
@@ -538,10 +539,10 @@ Histogram::Histogram(const std::string& name,
       declared_min_(minimum),
       declared_max_(maximum) {
   if (ranges) {
-    samples_.reset(
+    unlogged_samples_.reset(
         new PersistentSampleVector(HashMetricName(name), ranges, meta, counts));
     logged_samples_.reset(new PersistentSampleVector(
-        samples_->id(), ranges, logged_meta, logged_counts));
+        unlogged_samples_->id(), ranges, logged_meta, logged_counts));
   }
 }
 
@@ -598,10 +599,16 @@ HistogramBase* Histogram::DeserializeInfoImpl(PickleIterator* iter) {
   return histogram;
 }
 
-std::unique_ptr<SampleVector> Histogram::SnapshotSampleVector() const {
+std::unique_ptr<SampleVector> Histogram::SnapshotAllSamples() const {
+  std::unique_ptr<SampleVector> samples = SnapshotUnloggedSamples();
+  samples->Add(*logged_samples_);
+  return samples;
+}
+
+std::unique_ptr<SampleVector> Histogram::SnapshotUnloggedSamples() const {
   std::unique_ptr<SampleVector> samples(
-      new SampleVector(samples_->id(), bucket_ranges()));
-  samples->Add(*samples_);
+      new SampleVector(unlogged_samples_->id(), bucket_ranges()));
+  samples->Add(*unlogged_samples_);
   return samples;
 }
 
@@ -610,7 +617,7 @@ void Histogram::WriteAsciiImpl(bool graph_it,
                                std::string* output) const {
   // Get local (stack) copies of all effectively volatile class data so that we
   // are consistent across our output activities.
-  std::unique_ptr<SampleVector> snapshot = SnapshotSampleVector();
+  std::unique_ptr<SampleVector> snapshot = SnapshotAllSamples();
   Count sample_count = snapshot->TotalCount();
 
   WriteAsciiHeader(*snapshot, sample_count, output);
@@ -722,7 +729,7 @@ void Histogram::GetParameters(DictionaryValue* params) const {
 void Histogram::GetCountAndBucketData(Count* count,
                                       int64_t* sum,
                                       ListValue* buckets) const {
-  std::unique_ptr<SampleVector> snapshot = SnapshotSampleVector();
+  std::unique_ptr<SampleVector> snapshot = SnapshotAllSamples();
   *count = snapshot->TotalCount();
   *sum = snapshot->sum();
   uint32_t index = 0;

base/metrics/histogram.h

@@ -264,8 +264,13 @@ class BASE_EXPORT Histogram : public HistogramBase {
       base::PickleIterator* iter);
   static HistogramBase* DeserializeInfoImpl(base::PickleIterator* iter);
 
-  // Implementation of SnapshotSamples function.
-  std::unique_ptr<SampleVector> SnapshotSampleVector() const;
+  // Create a snapshot containing all samples (both logged and unlogged).
+  // Implementation of SnapshotSamples method with a more specific type for
+  // internal use.
+  std::unique_ptr<SampleVector> SnapshotAllSamples() const;
+
+  // Create a copy of unlogged samples.
+  std::unique_ptr<SampleVector> SnapshotUnloggedSamples() const;
 
   //----------------------------------------------------------------------------
   // Helpers for emitting Ascii graphic.  Each method appends data to output.
@@ -303,11 +308,10 @@ class BASE_EXPORT Histogram : public HistogramBase {
   Sample declared_min_;  // Less than this goes into the first bucket.
   Sample declared_max_;  // Over this goes into the last bucket.
 
-  // Finally, provide the state that changes with the addition of each new
-  // sample.
-  std::unique_ptr<SampleVectorBase> samples_;
+  // Samples that have not yet been logged with SnapshotDelta().
+  std::unique_ptr<HistogramSamples> unlogged_samples_;
 
-  // Also keep a previous uploaded state for calculating deltas.
+  // Accumulation of all samples that have been logged with SnapshotDelta().
   std::unique_ptr<HistogramSamples> logged_samples_;
 
   // Flag to indicate if PrepareFinalDelta has been previously called. It is

base/metrics/histogram_base.h

@@ -187,6 +187,9 @@ class BASE_EXPORT HistogramBase {
 
   // Snapshot the current complete set of sample data.
   // Override with atomic/locked snapshot if needed.
+  // NOTE: this data can overflow for long-running sessions. It should be
+  // handled with care and this method is recommended to be used only
+  // in about:histograms and test code.
   virtual std::unique_ptr<HistogramSamples> SnapshotSamples() const = 0;
 
   // Calculate the change (delta) in histogram counts since the previous call

base/metrics/histogram_unittest.cc

@@ -401,6 +401,26 @@ TEST_P(HistogramTest, AddCount_LargeValuesDontOverflow) {
   EXPECT_EQ(19400000000LL, samples2->sum());
 }
 
+// Some metrics are designed so that they are guaranteed not to overflow between
+// snapshots, but could overflow over a long-running session.
+// Make sure that counts returned by Histogram::SnapshotDelta do not overflow
+// even when a total count (returned by Histogram::SnapshotSample) does.
+TEST_P(HistogramTest, AddCount_LargeCountsDontOverflow) {
+  const size_t kBucketCount = 10;
+  Histogram* histogram = static_cast<Histogram*>(Histogram::FactoryGet(
+      "AddCountHistogram", 10, 50, kBucketCount, HistogramBase::kNoFlags));
+
+  const int count = (1 << 30) - 1;
+
+  // Repeat N times to make sure that there is no internal value overflow.
+  for (int i = 0; i < 10; ++i) {
+    histogram->AddCount(42, count);
+    std::unique_ptr<HistogramSamples> samples = histogram->SnapshotDelta();
+    EXPECT_EQ(count, samples->TotalCount());
+    EXPECT_EQ(count, samples->GetCount(42));
+  }
+}
+
 // Make sure histogram handles out-of-bounds data gracefully.
 TEST_P(HistogramTest, BoundsTest) {
   const size_t kBucketCount = 50;
@@ -416,7 +436,7 @@ TEST_P(HistogramTest, BoundsTest) {
   histogram->Add(10000);
 
   // Verify they landed in the underflow, and overflow buckets.
-  std::unique_ptr<SampleVector> samples = histogram->SnapshotSampleVector();
+  std::unique_ptr<SampleVector> samples = histogram->SnapshotAllSamples();
   EXPECT_EQ(2, samples->GetCountAtIndex(0));
   EXPECT_EQ(0, samples->GetCountAtIndex(1));
   size_t array_size = histogram->bucket_count();
@@ -441,7 +461,7 @@ TEST_P(HistogramTest, BoundsTest) {
 
   // Verify they landed in the underflow, and overflow buckets.
   std::unique_ptr<SampleVector> custom_samples =
-      test_custom_histogram->SnapshotSampleVector();
+      test_custom_histogram->SnapshotAllSamples();
   EXPECT_EQ(2, custom_samples->GetCountAtIndex(0));
   EXPECT_EQ(0, custom_samples->GetCountAtIndex(1));
   size_t bucket_count = test_custom_histogram->bucket_count();
@@ -464,7 +484,7 @@ TEST_P(HistogramTest, BucketPlacementTest) {
   }
 
   // Check to see that the bucket counts reflect our additions.
-  std::unique_ptr<SampleVector> samples = histogram->SnapshotSampleVector();
+  std::unique_ptr<SampleVector> samples = histogram->SnapshotAllSamples();
   for (int i = 0; i < 8; i++)
     EXPECT_EQ(i + 1, samples->GetCountAtIndex(i));
 }
@@ -484,7 +504,7 @@ TEST_P(HistogramTest, CorruptSampleCounts) {
   histogram->Add(20);
   histogram->Add(40);
 
-  std::unique_ptr<SampleVector> snapshot = histogram->SnapshotSampleVector();
+  std::unique_ptr<SampleVector> snapshot = histogram->SnapshotAllSamples();
   EXPECT_EQ(HistogramBase::NO_INCONSISTENCIES,
             histogram->FindCorruption(*snapshot));
   EXPECT_EQ(2, snapshot->redundant_count());

base/metrics/sparse_histogram.cc

@@ -85,7 +85,7 @@ std::unique_ptr<HistogramBase> SparseHistogram::PersistentCreate(
 SparseHistogram::~SparseHistogram() {}
 
 uint64_t SparseHistogram::name_hash() const {
-  return samples_->id();
+  return unlogged_samples_->id();
 }
 
 HistogramType SparseHistogram::GetHistogramType() const {
@@ -111,7 +111,7 @@ void SparseHistogram::AddCount(Sample value, int count) {
   }
   {
     base::AutoLock auto_lock(lock_);
-    samples_->Accumulate(value, count);
+    unlogged_samples_->Accumulate(value, count);
   }
 
   FindAndRunCallback(value);
@@ -121,7 +121,8 @@ std::unique_ptr<HistogramSamples> SparseHistogram::SnapshotSamples() const {
   std::unique_ptr<SampleMap> snapshot(new SampleMap(name_hash()));
 
   base::AutoLock auto_lock(lock_);
-  snapshot->Add(*samples_);
+  snapshot->Add(*unlogged_samples_);
+  snapshot->Add(*logged_samples_);
   return std::move(snapshot);
 }
 
@@ -130,10 +131,9 @@ std::unique_ptr<HistogramSamples> SparseHistogram::SnapshotDelta() {
 
   std::unique_ptr<SampleMap> snapshot(new SampleMap(name_hash()));
   base::AutoLock auto_lock(lock_);
-  snapshot->Add(*samples_);
+  snapshot->Add(*unlogged_samples_);
 
-  // Subtract what was previously logged and update that information.
-  snapshot->Subtract(*logged_samples_);
+  unlogged_samples_->Subtract(*snapshot);
   logged_samples_->Add(*snapshot);
   return std::move(snapshot);
 }
@@ -144,21 +144,19 @@ std::unique_ptr<HistogramSamples> SparseHistogram::SnapshotFinalDelta() const {
 
   std::unique_ptr<SampleMap> snapshot(new SampleMap(name_hash()));
   base::AutoLock auto_lock(lock_);
-  snapshot->Add(*samples_);
+  snapshot->Add(*unlogged_samples_);
 
-  // Subtract what was previously logged and then return.
-  snapshot->Subtract(*logged_samples_);
   return std::move(snapshot);
 }
 
 void SparseHistogram::AddSamples(const HistogramSamples& samples) {
   base::AutoLock auto_lock(lock_);
-  samples_->Add(samples);
+  unlogged_samples_->Add(samples);
 }
 
 bool SparseHistogram::AddSamplesFromPickle(PickleIterator* iter) {
   base::AutoLock auto_lock(lock_);
-  return samples_->AddFromPickle(iter);
+  return unlogged_samples_->AddFromPickle(iter);
 }
 
 void SparseHistogram::WriteHTMLGraph(std::string* output) const {
@@ -177,8 +175,8 @@ bool SparseHistogram::SerializeInfoImpl(Pickle* pickle) const {
 
 SparseHistogram::SparseHistogram(const std::string& name)
     : HistogramBase(name),
-      samples_(new SampleMap(HashMetricName(name))),
-      logged_samples_(new SampleMap(samples_->id())) {}
+      unlogged_samples_(new SampleMap(HashMetricName(name))),
+      logged_samples_(new SampleMap(unlogged_samples_->id())) {}
 
 SparseHistogram::SparseHistogram(PersistentHistogramAllocator* allocator,
                                  const std::string& name,
@@ -195,10 +193,11 @@ SparseHistogram::SparseHistogram(PersistentHistogramAllocator* allocator,
       // "active" samples use, for convenience purposes, an ID matching
       // that of the histogram while the "logged" samples use that number
       // plus 1.
-      samples_(new PersistentSampleMap(HashMetricName(name), allocator, meta)),
-      logged_samples_(
-          new PersistentSampleMap(samples_->id() + 1, allocator, logged_meta)) {
-}
+      unlogged_samples_(
+          new PersistentSampleMap(HashMetricName(name), allocator, meta)),
+      logged_samples_(new PersistentSampleMap(unlogged_samples_->id() + 1,
+                                              allocator,
+                                              logged_meta)) {}
 
 HistogramBase* SparseHistogram::DeserializeInfoImpl(PickleIterator* iter) {
   std::string histogram_name;

base/metrics/sparse_histogram.h

@@ -97,7 +97,7 @@ class BASE_EXPORT SparseHistogram : public HistogramBase {
   // Flag to indicate if PrepareFinalDelta has been previously called.
   mutable bool final_delta_created_ = false;
 
-  std::unique_ptr<HistogramSamples> samples_;
+  std::unique_ptr<HistogramSamples> unlogged_samples_;
   std::unique_ptr<HistogramSamples> logged_samples_;
 
   DISALLOW_COPY_AND_ASSIGN(SparseHistogram);

base/metrics/sparse_histogram_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/metrics/histogram_base.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/histogram_samples.h"
+#include "base/metrics/metrics_hashes.h"
 #include "base/metrics/persistent_histogram_allocator.h"
 #include "base/metrics/persistent_memory_allocator.h"
 #include "base/metrics/sample_map.h"
@@ -149,6 +150,25 @@ TEST_P(SparseHistogramTest, AddCount_LargeValuesDontOverflow) {
   EXPECT_EQ(55250000000LL, snapshot2->sum());
 }
 
+// Make sure that counts returned by Histogram::SnapshotDelta do not overflow
+// even when a total count (returned by Histogram::SnapshotSample) does.
+TEST_P(SparseHistogramTest, AddCount_LargeCountsDontOverflow) {
+  std::unique_ptr<SparseHistogram> histogram(NewSparseHistogram("Sparse"));
+  std::unique_ptr<HistogramSamples> snapshot(histogram->SnapshotSamples());
+  EXPECT_EQ(0, snapshot->TotalCount());
+  EXPECT_EQ(0, snapshot->sum());
+
+  const int count = (1 << 30) - 1;
+
+  // Repeat N times to make sure that there is no internal value overflow.
+  for (int i = 0; i < 10; ++i) {
+    histogram->AddCount(42, count);
+    std::unique_ptr<HistogramSamples> samples = histogram->SnapshotDelta();
+    EXPECT_EQ(count, samples->TotalCount());
+    EXPECT_EQ(count, samples->GetCount(42));
+  }
+}
+
 TEST_P(SparseHistogramTest, MacroBasicTest) {
   UMA_HISTOGRAM_SPARSE_SLOWLY("Sparse", 100);
   UMA_HISTOGRAM_SPARSE_SLOWLY("Sparse", 200);
@@ -364,4 +384,11 @@ TEST_P(SparseHistogramTest, ExtremeValues) {
   }
 }
 
+TEST_P(SparseHistogramTest, HistogramNameHash) {
+  const char kName[] = "TestName";
+  HistogramBase* histogram = SparseHistogram::FactoryGet(
+      kName, HistogramBase::kUmaTargetedHistogramFlag);
+  EXPECT_EQ(histogram->name_hash(), HashMetricName(kName));
+}
+
 }  // namespace base