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 152603)
+++ 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/08/23 07:50:54]

Why is this DCHECK no longer appropriate?

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

There's no "size" anymore. It's just some duplicated information and does not
work with sparse histogram.

 
   // 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");
-      WriteIntAttribute("min", histogram.ranges(i));
-      WriteIntAttribute("max", histogram.ranges(i + 1));
-      WriteIntAttribute("count", snapshot.counts(i));
-    }
+  HistogramBase::Sample min;
+  HistogramBase::Sample max;
+  HistogramBase::Count count;

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

nit: These should be probably be scoped within the for loop.

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

Done.

+  for (scoped_ptr<SampleCountIterator> it = snapshot.Iterator();
+       !it->Done();
+       it->Next()) {
+    it->Get(&min, &max, &count);
+    WriteIntAttribute("min", min);
+    WriteIntAttribute("max", max);
+    WriteIntAttribute("count", count);
   }
 
   // Write the protobuf version.
@@ -485,13 +492,16 @@
   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()) {
+    it->Get(&min, &max, &count);
+    HistogramEventProto::Bucket* bucket = histogram_proto->add_bucket();
+    bucket->set_min(min);
+    bucket->set_max(max);
+
+    // TODO(kaiwang): do we need bucket_index field?

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

Jim, are we realistically ever going to use this field, or is it ok to drop?

+
+    bucket->set_count(count);
   }
 }