[histogram] Make histograms more resistant to overflows.

base/metrics/sparse_histogram.cc

Index: base/metrics/sparse_histogram.cc
diff --git a/base/metrics/sparse_histogram.cc b/base/metrics/sparse_histogram.cc
index 916dcd5739ef845e824102b8f648dd43c62aeed8..edc34c7fe91068169fa6090bb9fc97dd5c83f013 100644
--- a/base/metrics/sparse_histogram.cc
+++ b/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 logged_samples_->id();

[Ilya Sherman, 2017/05/11 00:42:34]

nit: Why logged_ rather than unlogged_ here?  It looks like they actually differ
by 1, so it should matter which one is chosen.  I take it there aren't any tests
that verify this behavior is correct?  (Or am I confused about something?)

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

Yes, you're correct. Note that this behaviour is very confusing (at least for
the reader unfamiliar with this code)  and it probably warrants a comments from
someone who understands what's going on.

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

I think it should be unlogged_samples to match previous behavior. Otherwise,
this is a change in behavior.

Since what was previously samples is now unlogged_samples. I believe they need
different ids because ids need to be unique and are what are stored in the
persistent allocator. However, name_hash() on the histogram object is what is
used to report to UMA - i.e. it's a cached version of HashMetricName(name).

So this looks like to be breaking that. Given no tests failed for this, please
add a test for this in this CL since you would otherwise be breaking this.

The test should verify that value returned by name_hash() corresponds to
HashMetricName(hist_name).

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

Yes, using logged_samples ID will cause breakage and mismatches.

The comment on line 188 explains why the IDs differ.

I have a plan (no timeline, just a plan) to change this to holding both samples
in the same allocation rather than the existing separate allocations.  That'll
mean only one ID and prevent another possible problem should it be possible to
allocate only one due to a full allocator.

 }
 
 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;