Merge multiple histogram snapshots into single one for reporting.

base/metrics/histogram_snapshot_manager.cc

Index: base/metrics/histogram_snapshot_manager.cc
diff --git a/base/metrics/histogram_snapshot_manager.cc b/base/metrics/histogram_snapshot_manager.cc
index 3d1c0d666a245050953d6899b9eb1423922f77ef..f641d44f1154bff8d15930b94f93caa93b1298d0 100644
--- a/base/metrics/histogram_snapshot_manager.cc
+++ b/base/metrics/histogram_snapshot_manager.cc
@@ -10,28 +10,63 @@
 #include "base/metrics/statistics_recorder.h"
 #include "base/stl_util.h"
 
+namespace {
+const int kNewInconsistency = (int)0x80000000;

[Alexei Svitkine (slow), 2016/02/09 19:46:51]

Needs a comment. 

[bcwhite, 2016/02/11 16:42:39]

Done.

+}  // namespace
+
 namespace base {
 
 HistogramSnapshotManager::HistogramSnapshotManager(
     HistogramFlattener* histogram_flattener)
-    : histogram_flattener_(histogram_flattener) {
+    : preparing_deltas_(false),
+      histogram_flattener_(histogram_flattener) {
   DCHECK(histogram_flattener_);
 }
 
 HistogramSnapshotManager::~HistogramSnapshotManager() {
-  STLDeleteValues(&logged_samples_);
 }
 
-void HistogramSnapshotManager::PrepareDelta(const HistogramBase& histogram) {
+void HistogramSnapshotManager::StartDeltas() {
+  // Ensure that start/finish calls to not get nested.
+  DCHECK(!preparing_deltas_);
+  preparing_deltas_ = true;
+
+#ifdef DEBUG
+  for (auto iter : known_histograms) {

[Alexei Svitkine (slow), 2016/02/09 19:46:51]

Nit: (const auto& entry : known_histogram)

[bcwhite, 2016/02/11 16:42:38]

Done.

+    CHECK(!iter->second.histogram);
+    CHECK(!iter->second.accumulated);
+    CHECK(!(iter->second.inconsistencies & kNewInconsistency));
+  }
+#endif
+}
+
+void HistogramSnapshotManager::PrepareDelta(HistogramBase* histogram) {
+  PrepareSamples(histogram, histogram->SnapshotDelta());
+}
+
+void HistogramSnapshotManager::PrepareOnce(const HistogramBase* histogram) {
+  PrepareSamples(histogram, histogram->SnapshotSamples());
+}
+
+void HistogramSnapshotManager::PrepareSamples(
+    const HistogramBase* histogram,
+    scoped_ptr<HistogramSamples> samples) {
   DCHECK(histogram_flattener_);
 
-  // Get up-to-date snapshot of sample stats.
-  scoped_ptr<HistogramSamples> snapshot(histogram.SnapshotSamples());
+  // Get information known about this histogram.
+  SampleInfo* sample_info = &known_histograms_[histogram->name_hash()];
+  if (sample_info->histogram) {
+    DCHECK_EQ(sample_info->histogram->histogram_name(),
+              histogram->histogram_name()) << "hash collision";
+  } else {
+    // First time this histogram has been seen; datafill.
+    sample_info->histogram = histogram;
+  }
 
   // Crash if we detect that our histograms have been overwritten.  This may be
   // a fair distance from the memory smasher, but we hope to correlate these
   // crashes with other events, such as plugins, or usage patterns, etc.
-  int corruption = histogram.FindCorruption(*snapshot);
+  int corruption = histogram->FindCorruption(*samples);
   if (HistogramBase::BUCKET_ORDER_ERROR & corruption) {
     // The checksum should have caught this, so crash separately if it didn't.
     CHECK_NE(0, HistogramBase::RANGE_CHECKSUM_ERROR & corruption);
@@ -43,39 +78,60 @@ void HistogramSnapshotManager::PrepareDelta(const HistogramBase& histogram) {
   // Note, at this point corruption can only be COUNT_HIGH_ERROR or
   // COUNT_LOW_ERROR and they never arise together, so we don't need to extract
   // bits from corruption.
-  const uint64_t histogram_hash = histogram.name_hash();
   if (corruption) {
-    DLOG(ERROR) << "Histogram: " << histogram.histogram_name()
-                << " has data corruption: " << corruption;
+    DLOG(ERROR) << "Histogram: \"" << histogram->histogram_name()
+                << "\" has data corruption: " << corruption;
     histogram_flattener_->InconsistencyDetected(
         static_cast<HistogramBase::Inconsistency>(corruption));
     // Don't record corrupt data to metrics services.
-    int old_corruption = inconsistencies_[histogram_hash];
+    const int old_corruption = sample_info->inconsistencies;
     if (old_corruption == (corruption | old_corruption))
       return;  // We've already seen this corruption for this histogram.
-    inconsistencies_[histogram_hash] |= corruption;
-    histogram_flattener_->UniqueInconsistencyDetected(
-        static_cast<HistogramBase::Inconsistency>(corruption));
+    sample_info->inconsistencies |= corruption | kNewInconsistency;

[Alexei Svitkine (slow), 2016/02/09 19:46:51]

Can kNewInconsistency be added to the existing enum? i.e. enum Inconsistency {}
in histogram_base.h?

[bcwhite, 2016/02/11 16:42:39]

I can but it's used only internally here.  It would have no meaning to anything
else using those values.  Is that your preference?

[Alexei Svitkine (slow), 2016/02/16 16:46:05]

Yeah, I think it's still worth adding to the enum - else someone looking at the
enum will not have the full story and may accidentally add the same value.

[bcwhite, 2016/02/16 19:55:59]

Done.

+    // TODO(bcwhite): Can we clear the inconsistency for future collection?
     return;
   }
 
-  HistogramSamples* to_log;
-  auto it = logged_samples_.find(histogram_hash);
-  if (it == logged_samples_.end()) {
-    to_log = snapshot.release();
-
-    // This histogram has not been logged before, add a new entry.
-    logged_samples_[histogram_hash] = to_log;
+  if (!sample_info->accumulated) {
+    // This histogram has not been seen before; add a new entry.
+    sample_info->accumulated = samples.release();
   } else {
-    HistogramSamples* already_logged = it->second;
-    InspectLoggedSamplesInconsistency(*snapshot, already_logged);
-    snapshot->Subtract(*already_logged);
-    already_logged->Add(*snapshot);
-    to_log = snapshot.get();
+    // There are previous values from this histogram; add them together.
+    sample_info->accumulated->Add(*samples);
+  }
+}
+
+void HistogramSnapshotManager::FinishDeltas() {

[Alexei Svitkine (slow), 2016/02/09 19:46:51]

Please order the function in the .cc file in the same order they're in the .h
file.

[bcwhite, 2016/02/11 16:42:39]

Done.

+  DCHECK(preparing_deltas_);
+
+  // Iterate over all known histograms to see what should be recorded.
+  for (auto& iter : known_histograms_) {
+    SampleInfo* sample_info = &iter.second;
+
+    // First, record any histograms in which corruption was detected.
+    if (sample_info->inconsistencies & kNewInconsistency) {
+      sample_info->inconsistencies &= ~kNewInconsistency;
+      histogram_flattener_->UniqueInconsistencyDetected(

[Alexei Svitkine (slow), 2016/02/09 19:46:51]

I don't understand why you moved it here rather than using the original logic.

[bcwhite, 2016/02/11 16:42:39]

If done during aggregation, multiple reports could be generated for the same
histogram name if multiple versions exist and multiple inconsistencies exist. 
Done here it will generate only one report for any given histogram name.

+          static_cast<HistogramBase::Inconsistency>(
+              sample_info->inconsistencies));
+    }
+
+    // Second, record actual accumulated deltas.
+    if (sample_info->accumulated) {
+      if (sample_info->accumulated->redundant_count() > 0) {
+        histogram_flattener_->RecordDelta(*sample_info->histogram,
+                                          *sample_info->accumulated);
+      }
+      delete sample_info->accumulated;
+      sample_info->accumulated = nullptr;
+    }
+
+    // The Histogram pointer must be cleared at this point because the owner
+    // is only required to keep it alive until FinishDeltas() completes.
+    sample_info->histogram = nullptr;
   }
 
-  if (to_log->TotalCount() > 0)
-    histogram_flattener_->RecordDelta(histogram, *to_log);
+  preparing_deltas_ = false;
 }
 
 void HistogramSnapshotManager::InspectLoggedSamplesInconsistency(