Record sample-count overflow in sample-vector.

This has been running only with sparse histograms for some time now and
has been helpful in finding metrics that overflow.  Now extend this to
check the remaining bulk of histograms.

While the original CL looked for many different reasons of acquiring a
negative sample count, it's now known that there is only a single
reason and so that is the only reason checked for here.

Bug: 682680
Change-Id: I96785436468fdbe34f4221caf7a79da299edeca2
Reviewed-on: https://chromium-review.googlesource.com/741004Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Brian White <bcwhite@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513059}

base/metrics/histogram_samples.cc

@@ -7,6 +7,7 @@
 #include <limits>
 
 #include "base/compiler_specific.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/numerics/safe_math.h"
 #include "base/pickle.h"
@@ -251,6 +252,16 @@ void HistogramSamples::IncreaseSumAndCount(int64_t sum,
   subtle::NoBarrier_AtomicIncrement(&meta_->redundant_count, count);
 }
 
+void HistogramSamples::RecordNegativeSample(NegativeSampleReason reason,
+                                            HistogramBase::Count increment) {
+  UMA_HISTOGRAM_ENUMERATION("UMA.NegativeSamples.Reason", reason,
+                            MAX_NEGATIVE_SAMPLE_REASONS);
+  UMA_HISTOGRAM_CUSTOM_COUNTS("UMA.NegativeSamples.Increment", increment, 1,
+                              1 << 30, 100);
+  UMA_HISTOGRAM_SPARSE_SLOWLY("UMA.NegativeSamples.Histogram",
+                              static_cast<int32_t>(id()));
+}
+
 SampleCountIterator::~SampleCountIterator() {}
 
 bool SampleCountIterator::GetBucketIndex(size_t* index) const {

base/metrics/histogram_samples.h

@@ -160,6 +160,19 @@ class BASE_EXPORT HistogramSamples {
   }
 
  protected:
+  enum NegativeSampleReason {
+    SAMPLES_HAVE_LOGGED_BUT_NOT_SAMPLE,
+    SAMPLES_SAMPLE_LESS_THAN_LOGGED,
+    SAMPLES_ADDED_NEGATIVE_COUNT,
+    SAMPLES_ADD_WENT_NEGATIVE,
+    SAMPLES_ADD_OVERFLOW,
+    SAMPLES_ACCUMULATE_NEGATIVE_COUNT,
+    SAMPLES_ACCUMULATE_WENT_NEGATIVE,
+    DEPRECATED_SAMPLES_ACCUMULATE_OVERFLOW,
+    SAMPLES_ACCUMULATE_OVERFLOW,
+    MAX_NEGATIVE_SAMPLE_REASONS
+  };
+
   // Based on |op| type, add or subtract sample counts data from the iterator.
   enum Operator { ADD, SUBTRACT };
   virtual bool AddSubtractImpl(SampleCountIterator* iter, Operator op) = 0;
@@ -174,6 +187,10 @@ class BASE_EXPORT HistogramSamples {
   // Atomically adjust the sum and redundant-count.
   void IncreaseSumAndCount(int64_t sum, HistogramBase::Count count);
 
+  // Record a negative-sample observation and the reason why.
+  void RecordNegativeSample(NegativeSampleReason reason,
+                            HistogramBase::Count increment);
+
   AtomicSingleSample& single_sample() { return meta_->single_sample; }
   const AtomicSingleSample& single_sample() const {
     return meta_->single_sample;

base/metrics/persistent_sample_map.cc

@@ -18,19 +18,6 @@ typedef HistogramBase::Sample Sample;
 
 namespace {
 
-enum NegativeSampleReason {
-  PERSISTENT_SPARSE_HAVE_LOGGED_BUT_NOT_SAMPLE,
-  PERSISTENT_SPARSE_SAMPLE_LESS_THAN_LOGGED,
-  PERSISTENT_SPARSE_ADDED_NEGATIVE_COUNT,
-  PERSISTENT_SPARSE_ADD_WENT_NEGATIVE,
-  PERSISTENT_SPARSE_ADD_OVERFLOW,
-  PERSISTENT_SPARSE_ACCUMULATE_NEGATIVE_COUNT,
-  PERSISTENT_SPARSE_ACCUMULATE_WENT_NEGATIVE,
-  DEPRECATED_PERSISTENT_SPARSE_ACCUMULATE_OVERFLOW,
-  PERSISTENT_SPARSE_ACCUMULATE_OVERFLOW,
-  MAX_NEGATIVE_SAMPLE_REASONS
-};
-
 // An iterator for going through a PersistentSampleMap. The logic here is
 // identical to that of SampleMapIterator but with different data structures.
 // Changes here likely need to be duplicated there.
@@ -125,27 +112,19 @@ void PersistentSampleMap::Accumulate(Sample value, Count count) {
 #if 0  // TODO(bcwhite) Re-enable efficient version after crbug.com/682680.
   *GetOrCreateSampleCountStorage(value) += count;
 #else
-  NegativeSampleReason reason = MAX_NEGATIVE_SAMPLE_REASONS;
   Count* local_count_ptr = GetOrCreateSampleCountStorage(value);
   if (count < 0) {
-    reason = PERSISTENT_SPARSE_ACCUMULATE_NEGATIVE_COUNT;
     if (*local_count_ptr < -count)
-      reason = PERSISTENT_SPARSE_ACCUMULATE_WENT_NEGATIVE;
+      RecordNegativeSample(SAMPLES_ACCUMULATE_WENT_NEGATIVE, -count);
+    else
+      RecordNegativeSample(SAMPLES_ACCUMULATE_NEGATIVE_COUNT, -count);
     *local_count_ptr += count;
   } else {
     Sample old_value = *local_count_ptr;
     Sample new_value = old_value + count;
     *local_count_ptr = new_value;
     if ((new_value >= 0) != (old_value >= 0))
-      reason = PERSISTENT_SPARSE_ACCUMULATE_OVERFLOW;
-  }
-  if (reason != MAX_NEGATIVE_SAMPLE_REASONS) {
-    UMA_HISTOGRAM_ENUMERATION("UMA.NegativeSamples.Reason", reason,
-                              MAX_NEGATIVE_SAMPLE_REASONS);
-    UMA_HISTOGRAM_CUSTOM_COUNTS("UMA.NegativeSamples.Increment", count, 1,
-                                1 << 30, 100);
-    UMA_HISTOGRAM_SPARSE_SLOWLY("UMA.NegativeSamples.Histogram",
-                                static_cast<int32_t>(id()));
+      RecordNegativeSample(SAMPLES_ACCUMULATE_OVERFLOW, count);
   }
 #endif
   IncreaseSumAndCount(strict_cast<int64_t>(count) * value, count);

base/metrics/sample_vector.cc

@@ -55,8 +55,14 @@ void SampleVectorBase::Accumulate(Sample value, Count count) {
   }
 
   // Handle the multi-sample case.
-  subtle::NoBarrier_AtomicIncrement(&counts()[bucket_index], count);
+  Count new_value =
+      subtle::NoBarrier_AtomicIncrement(&counts()[bucket_index], count);
   IncreaseSumAndCount(strict_cast<int64_t>(count) * value, count);
+
+  // TODO(bcwhite) Remove after crbug.com/682680.
+  Count old_value = new_value - count;
+  if ((new_value >= 0) != (old_value >= 0) && count > 0)
+    RecordNegativeSample(SAMPLES_ACCUMULATE_OVERFLOW, count);
 }
 
 Count SampleVectorBase::GetCount(Sample value) const {

tools/metrics/histograms/enums.xml

@@ -27793,32 +27793,22 @@ from previous Chrome versions.
 </enum>
 
 <enum name="NegativeSampleReason">
-  <int value="0"
-      label="Persistent sparse histogram had logged value but no active
-             sample."/>
-  <int value="1"
-      label="Persistent sparse histogram active sample less than logged
-             value."/>
-  <int value="2"
-      label="Persistent sparse histogram added a negative count during
-             iteration."/>
+  <int value="0" label="Histogram had logged value but no active sample."/>
+  <int value="1" label="Histogram active sample less than logged value."/>
+  <int value="2" label="Histogram added a negative count during iteration."/>
   <int value="3"
-      label="Persistent sparse histogram added negative count that caused
-             negative value."/>
+      label="Histogram added negative count that caused negative value."/>
   <int value="4"
-      label="Persistent sparse histogram active sample overflowed and became
-             negative."/>
-  <int value="5"
-      label="Persistent sparse histogram accumulated a negative count."/>
+      label="Histogram active sample overflowed and became negative."/>
+  <int value="5" label="Histogram accumulated a negative count."/>
   <int value="6"
-      label="Persistent sparse histogram accumulated a negative count that
-             caused a negative value."/>
+      label="Histogram accumulated a negative count that caused a negative
+             value."/>
   <int value="7"
-      label="Persistent sparse histogram active sample was negative after
-             accumulation (deprecated)."/>
+      label="Histogram active sample was negative after accumulation
+             (deprecated)."/>
   <int value="8"
-      label="Persistent sparse histogram active sample wrapped 2^31 during
-             accumulation."/>
+      label="Histogram active sample wrapped 2^31 during accumulation."/>
 </enum>
 
 <enum name="NetCacheState">