SampleSet -> HistogramSamples (will be reused by SparseHistogram)

chrome/common/metrics/metrics_log_base.cc

Index: chrome/common/metrics/metrics_log_base.cc
===================================================================
--- chrome/common/metrics/metrics_log_base.cc	(revision 155400)
+++ chrome/common/metrics/metrics_log_base.cc	(working copy)
@@ -7,6 +7,8 @@
 #include "base/base64.h"
 #include "base/basictypes.h"
 #include "base/md5.h"
+#include "base/metrics/histogram_base.h"
+#include "base/metrics/histogram_samples.h"
 #include "base/perftimer.h"
 #include "base/string_number_conversions.h"
 #include "base/sys_byteorder.h"
@@ -23,6 +25,9 @@
 #define OPEN_ELEMENT_FOR_SCOPE(name) ScopedElement scoped_element(this, name)
 
 using base::Histogram;
+using base::HistogramBase;
+using base::HistogramSamples;
+using base::SampleCountIterator;
 using base::Time;
 using base::TimeDelta;
 using metrics::HistogramEventProto;
@@ -449,10 +454,9 @@
 // the same infrastructure for logging StatsCounters, RatesCounters, etc.
 void MetricsLogBase::RecordHistogramDelta(
     const Histogram& histogram,
-    const Histogram::SampleSet& snapshot) {
+    const HistogramSamples& snapshot) {
   DCHECK(!locked_);
   DCHECK_NE(0, snapshot.TotalCount());
-  DCHECK_EQ(histogram.bucket_count(), snapshot.size());

[Ilya Sherman, 2012/09/12 03:20:58]

nit: Can this DCHECK be restored?

[kaiwang, 2012/09/20 22:54:59]

now it's always true (enforced by HistogramSamples, which is created based on
bucket_count)

[Ilya Sherman, 2012/09/21 08:52:12]

Sorry, I don't follow.  These are being passed in as separate arguments --
nothing guarantees that they are for the same histogram...  What am I missing?

[kaiwang, 2012/09/21 19:51:18]

Sorry I didn't make it clear.
HistogramSamples does not have size() member function.
For this function, it actually outputs information separately from |histogram|
and |snapshot|. It's not a problem when snapshot is not from histogram. If
snapshot is from histogram, other code in Histogram enforces bucket_count()
always equals size()

 
   // We will ignore the MAX_INT/infinite value in the last element of range[].
 
@@ -471,13 +475,16 @@
   // TODO(jar): Remove sumsquares when protobuffer accepts this as optional.
   WriteInt64Attribute("sumsquares", 0);
 
-  for (size_t i = 0; i < histogram.bucket_count(); i++) {
-    if (snapshot.counts(i)) {
-      OPEN_ELEMENT_FOR_SCOPE("histogrambucket");

[Ilya Sherman, 2012/09/12 03:20:58]

What happened to this line?

[kaiwang, 2012/09/20 22:54:59]

Done.

-      WriteIntAttribute("min", histogram.ranges(i));
-      WriteIntAttribute("max", histogram.ranges(i + 1));
-      WriteIntAttribute("count", snapshot.counts(i));
-    }
+  for (scoped_ptr<SampleCountIterator> it = snapshot.Iterator();
+       !it->Done();
+       it->Next()) {
+    HistogramBase::Sample min;
+    HistogramBase::Sample max;
+    HistogramBase::Count count;
+    it->Get(&min, &max, &count);
+    WriteIntAttribute("min", min);
+    WriteIntAttribute("max", max);
+    WriteIntAttribute("count", count);
   }
 
   // Write the protobuf version.
@@ -485,13 +492,20 @@
   histogram_proto->set_name_hash(numeric_name_hash);
   histogram_proto->set_sum(snapshot.sum());
 
-  for (size_t i = 0; i < histogram.bucket_count(); ++i) {
-    if (snapshot.counts(i)) {
-      HistogramEventProto::Bucket* bucket = histogram_proto->add_bucket();
-      bucket->set_min(histogram.ranges(i));
-      bucket->set_max(histogram.ranges(i + 1));
-      bucket->set_bucket_index(i);
-      bucket->set_count(snapshot.counts(i));
-    }
+  for (scoped_ptr<SampleCountIterator> it = snapshot.Iterator();
+       !it->Done();
+       it->Next()) {
+    HistogramBase::Sample min;
+    HistogramBase::Sample max;
+    HistogramBase::Count count;
+    it->Get(&min, &max, &count);
+    HistogramEventProto::Bucket* bucket = histogram_proto->add_bucket();
+    bucket->set_min(min);
+    bucket->set_max(max);
+    bucket->set_count(count);
+
+    size_t index;
+    if (it->GetBucketIndex(&index))
+      bucket->set_bucket_index(index);
   }
 }