Merge multiple histogram snapshots into single one for reporting.

base/metrics/histogram_snapshot_manager.cc

 // Copyright (c) 2012 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/histogram_snapshot_manager.h"
 
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram_flattener.h"
 #include "base/metrics/histogram_samples.h"
 #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);
     CHECK(false);  // Crash for the bucket order corruption.
   }
   // Checksum corruption might not have caused order corruption.
   CHECK_EQ(0, HistogramBase::RANGE_CHECKSUM_ERROR & corruption);
 
   // 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();
+  if (!sample_info->accumulated) {
+    // This histogram has not been seen before; add a new entry.
+    sample_info->accumulated = samples.release();
+  } else {
+    // There are previous values from this histogram; add them together.
+    sample_info->accumulated->Add(*samples);
+  }
+}
 
-    // This histogram has not been logged before, add a new entry.
-    logged_samples_[histogram_hash] = to_log;
-  } else {
-    HistogramSamples* already_logged = it->second;
-    InspectLoggedSamplesInconsistency(*snapshot, already_logged);
-    snapshot->Subtract(*already_logged);
-    already_logged->Add(*snapshot);
-    to_log = snapshot.get();
+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(
       const HistogramSamples& new_snapshot,
       HistogramSamples* logged_samples) {
   HistogramBase::Count discrepancy =
       logged_samples->TotalCount() - logged_samples->redundant_count();
   if (!discrepancy)
     return;
 
   histogram_flattener_->InconsistencyDetectedInLoggedCount(discrepancy);
   if (discrepancy > Histogram::kCommonRaceBasedCountMismatch) {
     // Fix logged_samples.
     logged_samples->Subtract(*logged_samples);
     logged_samples->Add(new_snapshot);
   }
 }
 
 }  // namespace base