SampleSet -> HistogramSamples (will be reused by SparseHistogram)

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/metrics/statistics_recorder.h"
+#include "base/metrics/histogram_samples.h"
+#include "base/stl_util.h"
 
-using base::Histogram;
-using base::StatisticsRecorder;
+using std::map;
+using std::string;
 
 namespace base {
 
 HistogramSnapshotManager::HistogramSnapshotManager(
     HistogramFlattener* histogram_flattener)
     : histogram_flattener_(histogram_flattener) {
   DCHECK(histogram_flattener_);
 }
 
-HistogramSnapshotManager::~HistogramSnapshotManager() {}
+HistogramSnapshotManager::~HistogramSnapshotManager() {
+  STLDeleteValues(&logged_samples_);
+}
 
 void HistogramSnapshotManager::PrepareDeltas(Histogram::Flags flag_to_set,
                                              bool record_only_uma) {
   StatisticsRecorder::Histograms histograms;
   StatisticsRecorder::GetHistograms(&histograms);
   for (StatisticsRecorder::Histograms::const_iterator it = histograms.begin();
        histograms.end() != it;
        ++it) {
     (*it)->SetFlags(flag_to_set);
     if (record_only_uma &&
         0 == ((*it)->flags() & Histogram::kUmaTargetedHistogramFlag))
       continue;
     PrepareDelta(**it);
   }
 }
 
 void HistogramSnapshotManager::PrepareDelta(const Histogram& histogram) {
   DCHECK(histogram_flattener_);
 
   // Get up-to-date snapshot of sample stats.
-  Histogram::SampleSet snapshot;
-  histogram.SnapshotSample(&snapshot);
+  scoped_ptr<HistogramSamples> snapshot(histogram.SnapshotSamples());
   const std::string& histogram_name = histogram.histogram_name();
 
-  int corruption = histogram.FindCorruption(snapshot);
+  int corruption = histogram.FindCorruption(*snapshot);
 
   // 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.
   if (Histogram::BUCKET_ORDER_ERROR & corruption) {
     // The checksum should have caught this, so crash separately if it didn't.
     CHECK_NE(0, Histogram::RANGE_CHECKSUM_ERROR & corruption);
     CHECK(false);  // Crash for the bucket order corruption.
   }
   // Checksum corruption might not have caused order corruption.
   CHECK_EQ(0, Histogram::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.
   if (corruption) {
-    NOTREACHED();
+    DLOG(ERROR) << "Histogram: " << histogram_name
+                << " has data corruption: " << corruption;
     histogram_flattener_->InconsistencyDetected(
         static_cast<Histogram::Inconsistencies>(corruption));
-    // Don't record corrupt data to metrics survices.
-    if (NULL == inconsistencies_.get())
-      inconsistencies_.reset(new ProblemMap);
-    int old_corruption = (*inconsistencies_)[histogram_name];
+    // Don't record corrupt data to metrics services.
+    int old_corruption = inconsistencies_[histogram_name];
     if (old_corruption == (corruption | old_corruption))
       return;  // We've already seen this corruption for this histogram.
-    (*inconsistencies_)[histogram_name] |= corruption;
+    inconsistencies_[histogram_name] |= corruption;
     histogram_flattener_->UniqueInconsistencyDetected(
         static_cast<Histogram::Inconsistencies>(corruption));
     return;
   }
 
-  // Find the already recorded stats, or create an empty set. Remove from our
-  // snapshot anything that we've already recorded.
-  LoggedSampleMap::iterator it = logged_samples_.find(histogram_name);
-  Histogram::SampleSet* already_logged;
-  if (logged_samples_.end() == it) {
-    // Add new entry
-    already_logged = &logged_samples_[histogram.histogram_name()];
-    // Complete initialization.
-    already_logged->Resize(histogram.bucket_count());
+  HistogramSamples* already_logged;
+  HistogramSamples* to_log;
+
+  map<string, HistogramSamples*>::iterator it =
+      logged_samples_.find(histogram_name);
+  if (it == logged_samples_.end()) {
+    // This histogram has not been logged before, add a new entry.
+    already_logged = snapshot.release();

[Ilya Sherman, 2012/08/29 08:48:16]

nit: It seems clearer to just assign to |to_log| here, and move the
|already_logged| variable into the else-stmt

[kaiwang, 2012/08/29 22:42:16]

Done.

+    logged_samples_[histogram_name] = already_logged;
+    to_log = already_logged;
   } else {
-    already_logged = &(it->second);
-    int64 discrepancy(already_logged->TotalCount() -
-                      already_logged->redundant_count());
-    if (discrepancy) {
-      NOTREACHED();  // Already_logged has become corrupt.

[Ilya Sherman, 2012/08/29 08:48:16]

Why did you remove this NOTREACHED()?

[kaiwang, 2012/08/29 22:42:16]

It's trying to handle DCHECK error, which is not recommended by style guide. And
this "NOTREACHED" situation is always reached in Chrome release

-      int problem = static_cast<int>(discrepancy);
-      if (problem != discrepancy)
-        problem = INT_MAX;
-      histogram_flattener_->InconsistencyDetectedInLoggedCount(problem);
-      // With no valid baseline, we'll act like we've recorded everything in our
-      // snapshot.
-      already_logged->Subtract(*already_logged);
-      already_logged->Add(snapshot);
-    }
-    // Deduct any stats we've already logged from our snapshot.
-    snapshot.Subtract(*already_logged);
+    already_logged = it->second;
+    InspectLoggedSamplesInconsistency(*snapshot, already_logged);
+    snapshot->Subtract(*already_logged);
+    already_logged->Add(*snapshot);
+    to_log = snapshot.get();
   }
 
-  // Snapshot now contains only a delta to what we've already_logged.
-  if (snapshot.redundant_count() > 0) {
-    histogram_flattener_->RecordDelta(histogram, snapshot);
-    // Add new data into our running total.
-    already_logged->Add(snapshot);
+  if (to_log->redundant_count() > 0) {
+    histogram_flattener_->RecordDelta(histogram, *to_log);
   }

[Ilya Sherman, 2012/08/29 08:48:16]

nit: No need for curly braces.

[kaiwang, 2012/08/29 22:42:16]

Done.

 }
+
+void HistogramSnapshotManager::InspectLoggedSamplesInconsistency(
+      const HistogramSamples& new_snapshot,
+      HistogramSamples* logged_samples) {
+  HistogramBase::Count discrepancy =
+      logged_samples->TotalCount() - logged_samples->redundant_count();
+  if (discrepancy) {

[Ilya Sherman, 2012/08/29 08:48:16]

Optional nit: You can reduce the level of indentation below by reversing this
condition and returning early.

[kaiwang, 2012/08/29 22:42:16]

Done.

+    histogram_flattener_->InconsistencyDetectedInLoggedCount(discrepancy);
+    if (discrepancy > Histogram::kCommonRaceBasedCountMismatch) {
+      // Fix logged_samples.
+      logged_samples->Subtract(*logged_samples);
+      logged_samples->Add(new_snapshot);
+    }
+  }
+}
+
 }  // namespace base