SampleSet -> HistogramSamples (will be reused by SparseHistogram)

base/metrics/histogram_snapshot_manager.cc

Index: base/metrics/histogram_snapshot_manager.cc
===================================================================
--- base/metrics/histogram_snapshot_manager.cc	(revision 152603)
+++ base/metrics/histogram_snapshot_manager.cc	(working copy)
@@ -5,9 +5,11 @@
 #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;

[Ilya Sherman, 2012/08/23 07:50:54]

nit: Please just use the full std:: qualified names in the code -- that seems
more consistent with the usual Chrome coding style.

[kaiwang, 2012/08/24 04:17:58]

I really don't think so. You can find a lot of "using ..." in Chromium code.
Every C++ programmer should be familiar with map, string, vector, etc. And
people should not define classes with the same name.
Keeping all the "std::" only makes lines longer and hard to read.
It's only in .cc file and has a limited scoped.

 
 namespace base {
 
@@ -17,7 +19,9 @@
   DCHECK(histogram_flattener_);
 }
 
-HistogramSnapshotManager::~HistogramSnapshotManager() {}
+HistogramSnapshotManager::~HistogramSnapshotManager() {
+  STLDeleteValues(&logged_samples_);
+}
 
 void HistogramSnapshotManager::PrepareDeltas(Histogram::Flags flag_to_set,
                                              bool record_only_uma) {
@@ -38,11 +42,10 @@
   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
@@ -59,54 +62,56 @@
   // 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

[Ilya Sherman, 2012/08/23 07:50:54]

nit: Should this be a DLOG(FATAL)?

[kaiwang, 2012/08/24 04:17:58]

DLOG(FATAL) is like DCHECK.
We generally should not handle DCHECKs. See:
http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

+                << " 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.

[Ilya Sherman, 2012/08/23 07:50:54]

nit: Please preserve (or improve upon) this comment.

-  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 is not logged before, add a new entry.

[Ilya Sherman, 2012/08/23 07:50:54]

nit: "is not logged before" -> "has not been logged before"

[kaiwang, 2012/08/24 04:17:58]

Done.

+    already_logged = snapshot.release();
+    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.
-      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.

[Ilya Sherman, 2012/08/23 07:50:54]

nit: Please preserve (or improve upon) this comment.

-    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/23 07:50:54]

I can't immediately tell if you're changing the behavior of this implementation,
or simply trying to clean it up.  Is it possible to maintain a more parallel
structure within this CL, and split the cleanup/changes out into a separate,
follow-up CL?

[kaiwang, 2012/08/24 04:17:58]

I didn't change the logic. The problem is because the signature change of
SnapshotSample(s), I have to rewrite the above function. Then it doesn't make
sense I keep the original bad style.

+
+void HistogramSnapshotManager::InspectLoggedSamplesInconsistency(
+      const HistogramSamples& new_snapshot,
+      HistogramSamples* logged_samples) {
+  HistogramBase::Count discrepancy =
+      logged_samples->TotalCount() - logged_samples->redundant_count();
+  if (discrepancy) {
+    histogram_flattener_->InconsistencyDetectedInLoggedCount(discrepancy);
+    if (discrepancy > Histogram::kCommonRaceBasedCountMismatch) {
+      // Fix logged_samples.
+      logged_samples->Subtract(*logged_samples);
+      logged_samples->Add(new_snapshot);
+    }
+  }
+}
+
 }  // namespace base