[histogram] Make histograms more resistant to overflows.

base/metrics/histogram.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Histogram is an object that aggregates statistics, and can summarize them in
 // various forms, including ASCII graphical, HTML, and numerically (as a
 // vector of numbers corresponding to each of the aggregating buckets).
 // See header file for details and examples.
 
 #include "base/metrics/histogram.h"
@@ skipping 396 matching lines @@
 
   if (!check_okay) {
     UMA_HISTOGRAM_SPARSE_SLOWLY("Histogram.BadConstructionArguments",
                                 static_cast<Sample>(HashMetricName(name)));
   }
 
   return check_okay;
 }
 
 uint64_t Histogram::name_hash() const {
-  return samples_->id();
+  return unlogged_samples_->id();
 }
 
 HistogramType Histogram::GetHistogramType() const {
   return HISTOGRAM;
 }
 
 bool Histogram::HasConstructionArguments(Sample expected_minimum,
                                          Sample expected_maximum,
                                          uint32_t expected_bucket_count) const {
   return ((expected_minimum == declared_min_) &&
@@ skipping 10 matching lines @@
   DCHECK_EQ(kSampleType_MAX, ranges(bucket_count()));
 
   if (value > kSampleType_MAX - 1)
     value = kSampleType_MAX - 1;
   if (value < 0)
     value = 0;
   if (count <= 0) {
     NOTREACHED();
     return;
   }
-  samples_->Accumulate(value, count);
+  unlogged_samples_->Accumulate(value, count);
 
   FindAndRunCallback(value);
 }
 
 std::unique_ptr<HistogramSamples> Histogram::SnapshotSamples() const {
   return SnapshotSampleVector();
 }
 
 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();
-  }
+  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);
 
-  // Subtract what was previously logged and update that information.
-  snapshot->Subtract(*logged_samples_);
-  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.
 void Histogram::WriteHTMLGraph(std::string* output) const {
   // TBD(jar) Write a nice HTML bar chart, with divs an mouse-overs etc.
   output->append("<PRE>");
   WriteAsciiImpl(true, "<br>", output);
   output->append("</PRE>");
 }
 
@@ skipping 12 matching lines @@
 }
 
 Histogram::Histogram(const std::string& name,
                      Sample minimum,
                      Sample maximum,
                      const BucketRanges* ranges)
   : HistogramBase(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,
                      Sample minimum,
                      Sample maximum,
                      const BucketRanges* ranges,
                      const DelayedPersistentAllocation& counts,
                      const DelayedPersistentAllocation& logged_counts,
                      HistogramSamples::Metadata* meta,
                      HistogramSamples::Metadata* logged_meta)
     : HistogramBase(name),
       bucket_ranges_(ranges),
       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));
   }
 }
 
 Histogram::~Histogram() {
 }
 
 bool Histogram::PrintEmptyBucket(uint32_t index) const {
   return true;
 }
 
@@ skipping 37 matching lines @@
       histogram_name, declared_min, declared_max, bucket_count, flags);
 
   if (!ValidateRangeChecksum(*histogram, range_checksum)) {
     // The serialized histogram might be corrupted.
     return NULL;
   }
   return histogram;
 }
 
 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;
+}
+
 void Histogram::WriteAsciiImpl(bool graph_it,
                                const std::string& newline,
                                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();
   Count sample_count = snapshot->TotalCount();
 
   WriteAsciiHeader(*snapshot, sample_count, output);
   output->append(newline);
@@ skipping 585 matching lines @@
     Sample sample = custom_ranges[i];
     if (sample < 0 || sample > HistogramBase::kSampleType_MAX - 1)
       return false;
     if (sample != 0)
       has_valid_range = true;
   }
   return has_valid_range;
 }
 
 }  // namespace base