[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..bba04c928cb3ca743dcc07e284ff3ac5a858bc24 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,7 +445,7 @@ void Histogram::AddCount(int value, int count) {
     NOTREACHED();
     return;
   }
-  samples_->Accumulate(value, count);
+  unlogged_samples_->Accumulate(value, count);
 
   FindAndRunCallback(value);
 }
@@ -457,17 +457,10 @@ std::unique_ptr<HistogramSamples> Histogram::SnapshotSamples() const {
 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_);
+  std::unique_ptr<HistogramSamples> snapshot = SnapshotUnloggedSamples();
+  unlogged_samples_->Subtract(*snapshot);

[Alexei Svitkine (slow), 2017/05/10 16:55:38]

So logic difference here was that before it was:
  1. Take snapshot.
  2. Subtract from snapshot logged samples
  3. Add to logged samples the snapshot
  4. Return snapshot

And now it's:
  1. Take unlogged snapshot
  2. Subtract from unlogged_samples the snapshot.
  3. Add to logged_samples the snapshot
  4. Return snapshot

This has the effect that unlogged_samples gets modified (via Subtract()) while
it may be being updated from other threads that may be logging histograms.
Maybe, this is OK - but it's a difference from before, where this code was only
*reading* that state and not modifying it (it was *modifying* logged_samples but
no one else was changing that behind the scenes).

So this makes me a bit worried about concurrent modifications around here. At
the very least, we should have a comment here discussing why this is OK. The
comment could describe what happens under those scenarios and why the end result
is still safe.

[altimin, 2017/05/11 12:38:33]

Please take a look.

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

It's okay in that everything is still "eventually consistent".  There is a brief
moment between lines 461 and 462 where the total count (logged + unlogged) has
gone down should a total be taken in parallel by about://histograms or whatever.

The sample counts themselves are protected by the atomic add/sub operations.

   logged_samples_->Add(*snapshot);
+
   return snapshot;
 }
 
@@ -475,20 +468,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 +509,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 +528,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));
   }
 }
 
@@ -599,9 +589,15 @@ HistogramBase* Histogram::DeserializeInfoImpl(PickleIterator* iter) {
 }
 
 std::unique_ptr<SampleVector> Histogram::SnapshotSampleVector() const {
-  std::unique_ptr<SampleVector> samples(
-      new SampleVector(samples_->id(), bucket_ranges()));
-  samples->Add(*samples_);
+  std::unique_ptr<SampleVector> samples = SnapshotUnloggedSamples();
+  samples->Add(*logged_samples_);
+  return samples;
+}
+
+std::unique_ptr<SampleVector> Histogram::SnapshotUnloggedSamples() const {
+  std::unique_ptr<SampleVector> samples =
+      base::MakeUnique<SampleVector>(unlogged_samples_->id(), bucket_ranges());

[Alexei Svitkine (slow), 2017/05/10 16:55:38]

Nit: base::MakeUnique doesn't seem to add much here since it's already declaring
the type of |samples| so I'd just use the previous syntax - passing new to the
std::unique_ptr ctor.

[altimin, 2017/05/11 12:38:33]

Done.

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

I've been told that its desirable to always use MakeUnique if possible as it
means someone doing a manual audit of unique_ptr+new invocations doesn't have to
check it.

+  samples->Add(*unlogged_samples_);
   return samples;
 }