Serialize/Deserialize support in HistogramBase

base/metrics/histogram.cc

Index: base/metrics/histogram.cc
diff --git a/base/metrics/histogram.cc b/base/metrics/histogram.cc
index 795ea3a832387c4bbaa1ddb8d101c94b5166f806..9f08a85ca63f0abb67ea2d6bf42d539a48b0454b 100644
--- a/base/metrics/histogram.cc
+++ b/base/metrics/histogram.cc
@@ -30,6 +30,54 @@ using std::vector;
 
 namespace base {
 
+namespace {
+
+bool ReadHistogramArguments(PickleIterator* iter,
+                            string* histogram_name,
+                            int* flags,
+                            int* declared_min,
+                            int* declared_max,
+                            uint64* bucket_count,
+                            uint32* range_checksum) {
+  if (!iter->ReadString(histogram_name) ||
+      !iter->ReadInt(flags) ||
+      !iter->ReadInt(declared_min) ||
+      !iter->ReadInt(declared_max) ||
+      !iter->ReadUInt64(bucket_count) ||
+      !iter->ReadUInt32(range_checksum)) {
+    DLOG(ERROR) << "Pickle error decoding Histogram: " << *histogram_name;
+    return false;
+  }
+
+  // Since these fields may have come from an untrusted renderer, do additional
+  // checks above and beyond those in Histogram::Initialize()
+  if (*declared_max <= 0 ||
+      *declared_min <= 0 ||
+      *declared_max < *declared_min ||
+      INT_MAX / sizeof(HistogramBase::Count) <= *bucket_count ||
+      *bucket_count < 2) {
+    DLOG(ERROR) << "Values error decoding Histogram: " << histogram_name;
+    return false;
+  }
+
+  // We use the arguments to find or create the local version of the histogram
+  // in this process. So need to clear IPC flag first.

[Ilya Sherman, 2012/12/29 00:17:30]

nit: "in this process. So need to clear IPC flag" -> "in this process, so we
need to clear the IPC flag"

[kaiwang, 2013/01/08 00:51:40]

Done.

+  DCHECK(*flags & HistogramBase::kIPCSerializationSourceFlag);
+  *flags &= ~HistogramBase::kIPCSerializationSourceFlag;
+
+  return true;
+}
+
+bool ValidateRangeChecksum(const HistogramBase* histogram,

[Ilya Sherman, 2012/12/29 00:17:30]

nit: Please pass by const-reference.

[kaiwang, 2013/01/08 00:51:40]

Done.

+                           uint32 range_checksum) {
+  const Histogram* casted_histogram =
+      static_cast<const Histogram*>(histogram);
+
+  return casted_histogram->bucket_ranges()->checksum() == range_checksum;
+}
+
+}  // namespace
+
 typedef HistogramBase::Count Count;
 typedef HistogramBase::Sample Sample;
 
@@ -153,117 +201,6 @@ void Histogram::AddBoolean(bool value) {
   DCHECK(false);
 }
 
-void Histogram::AddSamples(const HistogramSamples& samples) {
-  samples_->Add(samples);
-}
-
-bool Histogram::AddSamplesFromPickle(PickleIterator* iter) {
-  return samples_->AddFromPickle(iter);
-}
-
-// static
-string Histogram::SerializeHistogramInfo(const Histogram& histogram,
-                                         const HistogramSamples& snapshot) {
-  DCHECK(histogram.bucket_ranges()->HasValidChecksum());

[Ilya Sherman, 2012/12/29 00:17:30]

What happened to this DCHECK?

[kaiwang, 2013/01/08 00:51:40]

Added to Histogram::SerializeInfoImpl

-
-  Pickle pickle;
-  pickle.WriteString(histogram.histogram_name());
-  pickle.WriteInt(histogram.declared_min());
-  pickle.WriteInt(histogram.declared_max());
-  pickle.WriteUInt64(histogram.bucket_count());
-  pickle.WriteUInt32(histogram.bucket_ranges()->checksum());
-  pickle.WriteInt(histogram.GetHistogramType());
-  pickle.WriteInt(histogram.flags());
-
-  histogram.SerializeRanges(&pickle);
-
-  snapshot.Serialize(&pickle);
-
-  return string(static_cast<const char*>(pickle.data()), pickle.size());
-}
-
-// static
-bool Histogram::DeserializeHistogramInfo(const string& histogram_info) {
-  if (histogram_info.empty()) {
-    return false;
-  }
-
-  Pickle pickle(histogram_info.data(),
-                static_cast<int>(histogram_info.size()));
-  string histogram_name;
-  int declared_min;
-  int declared_max;
-  uint64 bucket_count;
-  uint32 range_checksum;
-  int histogram_type;
-  int pickle_flags;
-
-  PickleIterator iter(pickle);
-  if (!iter.ReadString(&histogram_name) ||
-      !iter.ReadInt(&declared_min) ||
-      !iter.ReadInt(&declared_max) ||
-      !iter.ReadUInt64(&bucket_count) ||
-      !iter.ReadUInt32(&range_checksum) ||
-      !iter.ReadInt(&histogram_type) ||
-      !iter.ReadInt(&pickle_flags)) {
-    DLOG(ERROR) << "Pickle error decoding Histogram: " << histogram_name;
-    return false;
-  }
-
-  DCHECK(pickle_flags & kIPCSerializationSourceFlag);
-  // Since these fields may have come from an untrusted renderer, do additional
-  // checks above and beyond those in Histogram::Initialize()
-  if (declared_max <= 0 || declared_min <= 0 || declared_max < declared_min ||
-      INT_MAX / sizeof(Count) <= bucket_count || bucket_count < 2) {
-    DLOG(ERROR) << "Values error decoding Histogram: " << histogram_name;
-    return false;
-  }
-
-  Flags flags = static_cast<Flags>(pickle_flags & ~kIPCSerializationSourceFlag);
-
-  Histogram* render_histogram(NULL);
-
-  if (histogram_type == HISTOGRAM) {
-    render_histogram = Histogram::FactoryGet(
-        histogram_name, declared_min, declared_max, bucket_count, flags);
-  } else if (histogram_type == LINEAR_HISTOGRAM) {
-    render_histogram = LinearHistogram::FactoryGet(
-        histogram_name, declared_min, declared_max, bucket_count, flags);
-  } else if (histogram_type == BOOLEAN_HISTOGRAM) {
-    render_histogram = BooleanHistogram::FactoryGet(histogram_name, flags);
-  } else if (histogram_type == CUSTOM_HISTOGRAM) {
-    vector<Sample> sample_ranges(bucket_count);
-    if (!CustomHistogram::DeserializeRanges(&iter, &sample_ranges)) {
-      DLOG(ERROR) << "Pickle error decoding ranges: " << histogram_name;
-      return false;
-    }
-    render_histogram =
-        CustomHistogram::FactoryGet(histogram_name, sample_ranges, flags);
-  } else {
-    DLOG(ERROR) << "Error Deserializing Histogram Unknown histogram_type: "
-                << histogram_type;
-    return false;
-  }
-
-  DCHECK_EQ(render_histogram->declared_min(), declared_min);
-  DCHECK_EQ(render_histogram->declared_max(), declared_max);
-  DCHECK_EQ(render_histogram->bucket_count(), bucket_count);
-  DCHECK_EQ(render_histogram->GetHistogramType(), histogram_type);
-
-  if (render_histogram->bucket_ranges()->checksum() != range_checksum) {
-    return false;
-  }
-
-  if (render_histogram->flags() & kIPCSerializationSourceFlag) {
-    DVLOG(1) << "Single process mode, histogram observed and not copied: "
-             << histogram_name;
-    return true;
-  }
-
-  DCHECK_EQ(flags & render_histogram->flags(), flags);

[Ilya Sherman, 2012/12/29 00:17:30]

nit: What happened to this DCHECK?

[kaiwang, 2013/01/08 00:51:40]

Actually I really don't understand what's this DCHECK is for... Could you
enlighten me?

[Ilya Sherman, 2013/01/08 22:31:53]

I'm also not sure; perhaps Jim can comment?  I think there are two
possibilities: (1) the code has evolved so that this is no longer needed, or (2)
this is checking something subtle, that we should continue to check.

-  return render_histogram->AddSamplesFromPickle(&iter);
-}
-
 // static
 const int Histogram::kCommonRaceBasedCountMismatch = 5;
 
@@ -363,6 +300,14 @@ scoped_ptr<HistogramSamples> Histogram::SnapshotSamples() const {
   return SnapshotSampleVector().PassAs<HistogramSamples>();
 }
 
+void Histogram::AddSamples(const HistogramSamples& samples) {
+  samples_->Add(samples);
+}
+
+bool Histogram::AddSamplesFromPickle(PickleIterator* iter) {
+  return samples_->AddFromPickle(iter);
+}
+
 // The following methods provide a graphical histogram display.
 void Histogram::WriteHTMLGraph(string* output) const {
   // TBD(jar) Write a nice HTML bar chart, with divs an mouse-overs etc.
@@ -375,6 +320,18 @@ void Histogram::WriteAscii(string* output) const {
   WriteAsciiImpl(true, "\n", output);
 }
 
+bool Histogram::SerializeInfoImpl(Pickle* pickle) const {
+  if (pickle->WriteString(histogram_name()) &&
+      pickle->WriteInt(flags()) &&

[Ilya Sherman, 2012/12/29 00:17:30]

I'm curious, why did you move flags to be written earlier in the pickle than it
had been?

[kaiwang, 2013/01/08 00:51:40]

histogram_name and flags are fields from HistogramBase

+      pickle->WriteInt(declared_min()) &&
+      pickle->WriteInt(declared_max()) &&
+      pickle->WriteUInt64(bucket_count()) &&
+      pickle->WriteUInt32(bucket_ranges()->checksum())) {
+    return true;
+  }
+  return false;

[Ilya Sherman, 2012/12/29 00:17:30]

Optional nit: IMO it's tidier to write this as

return pickle->WriteString(histogram_name()) && ...

[kaiwang, 2013/01/08 00:51:40]

Done.

+}
+
 Histogram::Histogram(const string& name,
                      Sample minimum,
                      Sample maximum,
@@ -397,10 +354,6 @@ Histogram::~Histogram() {
   }
 }
 
-bool Histogram::SerializeRanges(Pickle* pickle) const {
-  return true;
-}
-
 bool Histogram::PrintEmptyBucket(size_t index) const {
   return true;
 }
@@ -431,6 +384,31 @@ const string Histogram::GetAsciiBucketRange(size_t i) const {
 //------------------------------------------------------------------------------
 // Private methods
 
+// static
+HistogramBase* Histogram::DeserializeHistogramInfo(PickleIterator* iter) {
+  string histogram_name;
+  int flags;
+  int declared_min;
+  int declared_max;
+  uint64 bucket_count;
+  uint32 range_checksum;
+
+  if (!ReadHistogramArguments(iter, &histogram_name, &flags, &declared_min,
+                              &declared_max, &bucket_count, &range_checksum)) {
+    return NULL;
+  }
+
+  // Find or create the local version of the histogram in this process.
+  HistogramBase* histogram = Histogram::FactoryGet(
+      histogram_name, declared_min, declared_max, bucket_count, flags);
+
+  if (!ValidateRangeChecksum(histogram, range_checksum)) {
+    // The serialized histogram might be corrupted.
+    return NULL;
+  }
+  return histogram;
+}
+
 scoped_ptr<SampleVector> Histogram::SnapshotSampleVector() const {
   scoped_ptr<SampleVector> samples(new SampleVector(bucket_ranges()));
   samples->Add(*samples_);
@@ -710,6 +688,29 @@ void LinearHistogram::InitializeBucketRanges(Sample minimum,
   ranges->ResetChecksum();
 }
 
+// static
+HistogramBase* LinearHistogram::DeserializeHistogramInfo(PickleIterator* iter) {
+  string histogram_name;
+  int flags;
+  int declared_min;
+  int declared_max;
+  uint64 bucket_count;
+  uint32 range_checksum;
+
+  if (!ReadHistogramArguments(iter, &histogram_name, &flags, &declared_min,
+                              &declared_max, &bucket_count, &range_checksum)) {
+    return NULL;
+  }
+
+  HistogramBase* histogram = LinearHistogram::FactoryGet(
+      histogram_name, declared_min, declared_max, bucket_count, flags);
+  if (!ValidateRangeChecksum(histogram, range_checksum)) {
+    // The serialized histogram might be corrupted.
+    return NULL;
+  }
+  return histogram;
+}
+
 //------------------------------------------------------------------------------
 // This section provides implementation for BooleanHistogram.
 //------------------------------------------------------------------------------
@@ -750,6 +751,29 @@ BooleanHistogram::BooleanHistogram(const string& name,
                                    const BucketRanges* ranges)
     : LinearHistogram(name, 1, 2, 3, ranges) {}
 
+HistogramBase* BooleanHistogram::DeserializeHistogramInfo(
+    PickleIterator* iter) {
+  string histogram_name;
+  int flags;
+  int declared_min;
+  int declared_max;
+  uint64 bucket_count;
+  uint32 range_checksum;
+
+  if (!ReadHistogramArguments(iter, &histogram_name, &flags, &declared_min,
+                              &declared_max, &bucket_count, &range_checksum)) {
+    return NULL;
+  }
+
+  HistogramBase* histogram = BooleanHistogram::FactoryGet(
+      histogram_name, flags);
+  if (!ValidateRangeChecksum(histogram, range_checksum)) {
+    // The serialized histogram might be corrupted.
+    return NULL;
+  }
+  return histogram;
+}

[Ilya Sherman, 2012/12/29 00:17:30]

It would be good to share more of this code, rather than repeating it three
times.  Why not move this logic into the method currently named
ReadHistogramArguments, and pass the histogram type as a parameter into that
method?  If the type is a custom histogram you'd include the block to
deserialize the ranges; otherwise, you'd just skip it.

[kaiwang, 2013/01/08 00:51:40]

I think the good thing of current code is it distributes the
serializing/deserializing code. (A good thing about polymorphism: limit
switch/case :) Or the function you suggested will look exactly like the old
Histogram::DeserializeHistogramInfo().

Please note I intentionly kept most of the serializing logic. E.g. declared_min,
declared_max are not needed in Boolean/CustomHistogram. The old big
Histogram::Deserialize function resulted in current situation that many
unnecessary fields get written and read.

[Ilya Sherman, 2013/01/08 22:31:53]

I'm not convinced that this is true: HistogramBase::DeserializeHistogramInfo()
still has a switch stmt in it.  IMO it would be better to just inline all of the
shared code into that function (or a helper function that that one calls),
rather than repeating it in so many places.

+
 //------------------------------------------------------------------------------
 // CustomHistogram:
 //------------------------------------------------------------------------------
@@ -809,26 +833,52 @@ CustomHistogram::CustomHistogram(const string& name,
                 ranges->size() - 1,
                 ranges) {}
 
-bool CustomHistogram::SerializeRanges(Pickle* pickle) const {
-  for (size_t i = 0; i < bucket_ranges()->size(); ++i) {
+bool CustomHistogram::SerializeInfoImpl(Pickle* pickle) const {
+  if (!Histogram::SerializeInfoImpl(pickle))
+    return false;
+
+  // Serialize ranges. First and last ranges are alwasy 0 and INT_MAX, so don't
+  // write them.
+  for (size_t i = 1; i < bucket_ranges()->size() - 1; ++i) {
     if (!pickle->WriteInt(bucket_ranges()->range(i)))
       return false;
   }
   return true;
 }
 
+double CustomHistogram::GetBucketSize(Count current, size_t i) const {
+  return 1;
+}
+
 // static
-bool CustomHistogram::DeserializeRanges(
-    PickleIterator* iter, vector<Sample>* ranges) {
-  for (size_t i = 0; i < ranges->size(); ++i) {
-    if (!iter->ReadInt(&(*ranges)[i]))
-      return false;
+HistogramBase* CustomHistogram::DeserializeHistogramInfo(PickleIterator* iter) {
+  string histogram_name;
+  int flags;
+  int declared_min;
+  int declared_max;
+  uint64 bucket_count;
+  uint32 range_checksum;
+
+  if (!ReadHistogramArguments(iter, &histogram_name, &flags, &declared_min,
+                              &declared_max, &bucket_count, &range_checksum)) {
+    return NULL;
   }
-  return true;
-}
 
-double CustomHistogram::GetBucketSize(Count current, size_t i) const {
-  return 1;
+  // First and last ranges are not serialized.
+  vector<Sample> sample_ranges(bucket_count - 1);
+
+  for (size_t i = 0; i < sample_ranges.size(); ++i) {
+    if (!iter->ReadInt(&sample_ranges[i]))
+      return NULL;
+  }
+
+  HistogramBase* histogram = CustomHistogram::FactoryGet(
+      histogram_name, sample_ranges, flags);
+  if (!ValidateRangeChecksum(histogram, range_checksum)) {
+    // The serialized histogram might be corrupted.
+    return NULL;
+  }
+  return histogram;
 }
 
 // static