Report reasons for a negative histogram sample count.

base/metrics/persistent_sample_map.cc

 // Copyright 2016 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.
 
 #include "base/metrics/persistent_sample_map.h"
 
 #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"
 
 namespace base {
 
 typedef HistogramBase::Count Count;
 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.
 class PersistentSampleMapIterator : public SampleCountIterator {
  public:
   typedef std::map<HistogramBase::Sample, HistogramBase::Count*>
       SampleToCountMap;
 
   explicit PersistentSampleMapIterator(const SampleToCountMap& sample_counts);
   ~PersistentSampleMapIterator() override;
@@ skipping 145 matching lines @@
   return ref;
 }
 
 bool PersistentSampleMap::AddSubtractImpl(SampleCountIterator* iter,
                                           Operator op) {
   Sample min;
   Sample max;
   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;
 }
 
 Count* PersistentSampleMap::GetSampleCountStorage(Sample value) {
   // If |value| is already in the map, just return that.
   auto it = sample_counts_.find(value);
   if (it != sample_counts_.end())
     return it->second;
 
@@ skipping 78 matching lines @@
         found_count = &record->count;
       if (!import_everything)
         break;
     }
   }
 
   return found_count;
 }
 
 }  // namespace base