Report reasons for a negative histogram sample count.

base/metrics/persistent_sample_map.cc

Index: base/metrics/persistent_sample_map.cc
diff --git a/base/metrics/persistent_sample_map.cc b/base/metrics/persistent_sample_map.cc
index 1cc425423c787dc4575ceaeed0640c8337ac59d0..63c0359cbc31b07df794a757b98985766e221d0f 100644
--- a/base/metrics/persistent_sample_map.cc
+++ b/base/metrics/persistent_sample_map.cc
@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/metrics/persistent_histogram_allocator.h"
 #include "base/stl_util.h"
 
@@ -16,6 +17,13 @@ typedef HistogramBase::Sample Sample;
 
 namespace {
 
+enum NegativeSampleReason {
+  NOT_NEGATIVE,

[Alexei Svitkine (slow), 2017/01/20 19:11:30]

Since you're never logging this, suggest just removing it and for the purposes
of the placeholder value using MAX_NEGATIVE_SAMPLE_REASONS.

[bcwhite, 2017/01/20 19:17:06]

I use it as a default value in order to be able set the reason through
conditions and then check later if that was done.

[Alexei Svitkine (slow), 2017/01/20 19:21:30]

Right, I am suggesting you could use MAX_NEGATIVE_SAMPLE_REASONS for the default
value in the same way.

[bcwhite, 2017/01/20 19:59:19]

Done.

+  PERSISTENT_SPARSE_HAVE_LOGGED_BUT_NOT_SAMPLE,
+  PERSISTENT_SPARSE_SAMPLE_LESS_THAN_LOGGED,
+  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.
@@ -181,11 +189,39 @@ bool PersistentSampleMap::AddSubtractImpl(SampleCountIterator* iter,
   Count count;
   for (; !iter->Done(); iter->Next()) {
     iter->Get(&min, &max, &count);
+    if (count == 0)
+      continue;
     if (min + 1 != max)
       return false;  // SparseHistogram only supports bucket with size 1.
 
+#if 0  // TODO(bcwhite) Re-enable efficient version after crbug.com/682680.
     *GetOrCreateSampleCountStorage(min) +=
         (op == HistogramSamples::ADD) ? count : -count;
+#else
+    if (op == HistogramSamples::ADD) {
+      *GetOrCreateSampleCountStorage(min) += count;
+    } else {
+      // Subtract is used only for determining deltas when reporting which
+      // means that it's in the "logged" iterator. It should have an active
+      // sample record and thus there is no need to try to create one.
+      NegativeSampleReason reason = NOT_NEGATIVE;
+      Count* bucket = GetSampleCountStorage(min);
+      if (bucket == nullptr) {
+        reason = PERSISTENT_SPARSE_HAVE_LOGGED_BUT_NOT_SAMPLE;
+      } else {
+        if (*bucket < count) {
+          reason = PERSISTENT_SPARSE_SAMPLE_LESS_THAN_LOGGED;
+          *bucket = 0;

[Alexei Svitkine (slow), 2017/01/20 19:11:30]

This is also changing behavior - right?

Seems it would be better to keep behavior as before (i.e. having the negatives)
so that we can debug this, no? Otherwise, it's hard to correlate the new
histogram to the actual "bad" data since you're getting rid of it?

[bcwhite, 2017/01/20 19:17:06]

Correct.  The benefit this way is that we'll know that any negative samples must
come from another source.  That seems more reliable than trying to correlate the
number of these reports with the number of negative samples.

[Alexei Svitkine (slow), 2017/01/20 19:21:30]

OK.

+        } else {
+          *bucket -= count;
+        }
+      }
+      if (reason != NOT_NEGATIVE) {
+        UMA_HISTOGRAM_ENUMERATION("UMA.NegativeSamples.Reason", reason,
+                                  MAX_NEGATIVE_SAMPLE_REASONS);
+      }
+    }
+#endif
   }
   return true;
 }