Monitor Add for negative results.

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 {
   PERSISTENT_SPARSE_HAVE_LOGGED_BUT_NOT_SAMPLE,
   PERSISTENT_SPARSE_SAMPLE_LESS_THAN_LOGGED,
+  PERSISTENT_SPARSE_ADDED_NEGATIVE_COUNT,
+  PERSISTENT_SPARSE_ADD_WENT_NEGATIVE,
   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;
@@ skipping 157 matching lines @@
     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
+    NegativeSampleReason reason = MAX_NEGATIVE_SAMPLE_REASONS;
     if (op == HistogramSamples::ADD) {
-      *GetOrCreateSampleCountStorage(min) += count;
+      // Add should generally be adding only positive values.
+      Count* bucket = GetOrCreateSampleCountStorage(min);

[Alexei Svitkine (slow), 2017/04/26 19:37:58]

Nit: call this bucket_value as |bucket| is often used to refer to bucket index.

[bcwhite, 2017/04/26 20:47:14]

Went with |local_count_ptr| to match with |count| value retrieved from iterator.

+      if (count < 0) {
+        reason = PERSISTENT_SPARSE_ADDED_NEGATIVE_COUNT;
+        if (*bucket < -count) {
+          reason = PERSISTENT_SPARSE_ADD_WENT_NEGATIVE;
+          *bucket = 0;
+        }
+      } else {
+        *bucket += count;

[Alexei Svitkine (slow), 2017/04/26 19:37:58]

Couldn't this result in a negative too? i.e. due to overflow.

Let's check for that too.

[bcwhite, 2017/04/26 20:47:14]

Done.

+      }
     } 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 = MAX_NEGATIVE_SAMPLE_REASONS;
       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;
         } else {
           *bucket -= count;
         }
       }
-      if (reason != MAX_NEGATIVE_SAMPLE_REASONS) {
-        UMA_HISTOGRAM_ENUMERATION("UMA.NegativeSamples.Reason", reason,
-                                  MAX_NEGATIVE_SAMPLE_REASONS);
-      }
+    }
+    if (reason != MAX_NEGATIVE_SAMPLE_REASONS) {
+      NOTREACHED();
+      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())
@@ skipping 80 matching lines @@
         found_count = &record->count;
       if (!import_everything)
         break;
     }
   }
 
   return found_count;
 }
 
 }  // namespace base