[histogram] Make histograms more resistant to overflows.

base/metrics/histogram.cc

Index: base/metrics/histogram.cc
diff --git a/base/metrics/histogram.cc b/base/metrics/histogram.cc
index c35f76ce5548d8f0264f26fbeefab9c5d62aea82..dff5f36599fe21fa45efa6d03d54b5af292d984f 100644
--- a/base/metrics/histogram.cc
+++ b/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,29 +445,30 @@ 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();
-  }
+  // This code takes unlogged samples, adds them to logged samples,
+  // resets them and returns them.
+  // Correctness of this code relies on unlogged_samples being thread-safe
+  // due to atomic integer operations. Snapshot is taken by atomically copying
+  // all existing samples. Reset is implemented by subtracting copied snapshot,
+  // leaving samples added concurrently from another thread in unlogged_samples
+  // to be reported by the next call to SnapshotDelta.

[Alexei Svitkine (slow), 2017/05/11 13:54:35]

Maybe call out the function names where the atomicity happens?

e.g. for Subtract(), it's in SampleVectorBase::AddSubtractImpl().

Maybe also worth discussing why it's OK with the single bucket optimization
(where we don't allocate the full counts array until a 2nd bucket is needed).
Basically, discuss the case where we can go from single bucket to full counts
array in the middle of any of these operations (including in the middle of
Subtract) - and clearly point out why that's correct.

bcwhite@ can answer questions about how that works.

[bcwhite, 2017/05/11 17:34:52]

Basically, every individual operation is atomic but there's no synchronization
between them, hence the "eventually consistent" design.  You might read 5 but
have it (externally) incremented to 50 before that count is removed and dropping
it to 45.

The snapshot sum/count could also be off because of timing between updates and
reads.  We just live with that.

The single-bucket optimization is contained in SampleVector and its transition
from one-sample to many-sample is also atomic and "eventually consistent".  At
the SampleVectorBase API, there is no apparent difference.

[altimin, 2017/05/11 17:55:43]

I think that does not belong here. We should mention that SampleVector::Subtract
and SampleVector::Accumulate are thread-safe and maybe add a comment in
SampleVector explaining the details.

 
-  // Subtract what was previously logged and update that information.
-  snapshot->Subtract(*logged_samples_);
+  std::unique_ptr<HistogramSamples> snapshot = SnapshotUnloggedSamples();
+  unlogged_samples_->Subtract(*snapshot);
   logged_samples_->Add(*snapshot);
+
   return snapshot;
 }
 
@@ -475,20 +476,15 @@ 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 +517,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 +536,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 +596,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 +614,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 +726,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;