Serialize/Deserialize support in HistogramBase

base/metrics/histogram.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.
 
 // Histogram is an object that aggregates statistics, and can summarize them in
 // various forms, including ASCII graphical, HTML, and numerically (as a
 // vector of numbers corresponding to each of the aggregating buckets).
 // See header file for details and examples.
 
 #include "base/metrics/histogram.h"
@@ skipping 12 matching lines @@
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/synchronization/lock.h"
 #include "base/values.h"
 
 using std::string;
 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;
 
 // static
 const size_t Histogram::kBucketCount_MAX = 16384u;
 
 // TODO(rtenneti): delete this code after debugging.
 void CheckCorruption(const Histogram& histogram, bool new_histogram) {
   const std::string& histogram_name = histogram.histogram_name();
   char histogram_name_buf[128];
@@ skipping 103 matching lines @@
     ranges->set_range(bucket_index, current);
   }
   ranges->set_range(ranges->size() - 1, HistogramBase::kSampleType_MAX);
   ranges->ResetChecksum();
 }
 
 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;
 
 Histogram::Inconsistencies Histogram::FindCorruption(
     const HistogramSamples& samples) const {
   int inconsistencies = NO_INCONSISTENCIES;
   Sample previous_range = -1;  // Bottom range is always 0.
   for (size_t index = 0; index < bucket_count(); ++index) {
     int new_range = ranges(index);
     if (previous_range >= new_range)
@@ skipping 79 matching lines @@
     value = kSampleType_MAX - 1;
   if (value < 0)
     value = 0;
   samples_->Accumulate(value, 1);
 }
 
 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.
   output->append("<PRE>");
   WriteAsciiImpl(true, "<br>", output);
   output->append("</PRE>");
 }
 
 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,
                      size_t bucket_count,
                      const BucketRanges* ranges)
   : HistogramBase(name),
     bucket_ranges_(ranges),
     declared_min_(minimum),
     declared_max_(maximum),
     bucket_count_(bucket_count) {
   if (ranges)
     samples_.reset(new SampleVector(ranges));
 }
 
 Histogram::~Histogram() {
   if (StatisticsRecorder::dump_on_exit()) {
     string output;
     WriteAsciiImpl(true, "\n", &output);
     DLOG(INFO) << output;
   }
 }
 
-bool Histogram::SerializeRanges(Pickle* pickle) const {
-  return true;
-}
-
 bool Histogram::PrintEmptyBucket(size_t index) const {
   return true;
 }
 
 // Use the actual bucket widths (like a linear histogram) until the widths get
 // over some transition value, and then use that transition width.  Exponentials
 // get so big so fast (and we don't expect to see a lot of entries in the large
 // buckets), so we need this to make it possible to see what is going on and
 // not have 0-graphical-height buckets.
 double Histogram::GetBucketSize(Count current, size_t i) const {
@@ skipping 10 matching lines @@
   if (kHexRangePrintingFlag & flags())
     StringAppendF(&result, "%#x", ranges(i));
   else
     StringAppendF(&result, "%d", ranges(i));
   return result;
 }
 
 //------------------------------------------------------------------------------
 // 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_);
   return samples.Pass();
 }
 
 void Histogram::WriteAsciiImpl(bool graph_it,
                                const string& newline,
                                string* output) const {
   // Get local (stack) copies of all effectively volatile class data so that we
@@ skipping 259 matching lines @@
   size_t i;
   for (i = 1; i < bucket_count; ++i) {
     double linear_range =
         (min * (bucket_count -1 - i) + max * (i - 1)) / (bucket_count - 2);
     ranges->set_range(i, static_cast<Sample>(linear_range + 0.5));
   }
   ranges->set_range(ranges->size() - 1, HistogramBase::kSampleType_MAX);
   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.
 //------------------------------------------------------------------------------
 
 Histogram* BooleanHistogram::FactoryGet(const string& name, int32 flags) {
   Histogram* histogram = StatisticsRecorder::FindHistogram(name);
   if (!histogram) {
     // To avoid racy destruction at shutdown, the following will be leaked.
     BucketRanges* ranges = new BucketRanges(4);
     LinearHistogram::InitializeBucketRanges(1, 2, 3, ranges);
@@ skipping 20 matching lines @@
 }
 
 void BooleanHistogram::AddBoolean(bool value) {
   Add(value ? 1 : 0);
 }
 
 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:
 //------------------------------------------------------------------------------
 
 Histogram* CustomHistogram::FactoryGet(const string& name,
                                        const vector<Sample>& custom_ranges,
                                        int32 flags) {
   CHECK(ValidateCustomRanges(custom_ranges));
 
   Histogram* histogram = StatisticsRecorder::FindHistogram(name);
@@ skipping 39 matching lines @@
 }
 
 CustomHistogram::CustomHistogram(const string& name,
                                  const BucketRanges* ranges)
     : Histogram(name,
                 ranges->range(1),
                 ranges->range(ranges->size() - 2),
                 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;
 }
 
-// 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;
-  }
-  return true;
-}
-
 double CustomHistogram::GetBucketSize(Count current, size_t i) const {
   return 1;
 }
 
 // static
+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;
+  }
+
+  // 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
 bool CustomHistogram::ValidateCustomRanges(
     const vector<Sample>& custom_ranges) {
   bool has_valid_range = false;
   for (size_t i = 0; i < custom_ranges.size(); i++) {
     Sample sample = custom_ranges[i];
     if (sample < 0 || sample > HistogramBase::kSampleType_MAX - 1)
       return false;
     if (sample != 0)
       has_valid_range = true;
   }
@@ skipping 12 matching lines @@
 
   BucketRanges* bucket_ranges = new BucketRanges(ranges.size());
   for (size_t i = 0; i < ranges.size(); i++) {
     bucket_ranges->set_range(i, ranges[i]);
   }
   bucket_ranges->ResetChecksum();
   return bucket_ranges;
 }
 
 }  // namespace base