Refactor of Histogram related code

base/metrics/histogram.h

Index: base/metrics/histogram.h
===================================================================
--- base/metrics/histogram.h	(revision 148055)
+++ base/metrics/histogram.h	(working copy)
@@ -9,14 +9,29 @@
 // It supports calls to accumulate either time intervals (which are processed
 // as integral number of milliseconds), or arbitrary integral units.
 
-// The default layout of buckets is exponential.  For example, buckets might
+// For Histogram(exponential histogram), LinearHistogram and CustomHistogram,
+// the minimal range is 1 (instead of 0), maximal range is

[jar (doing other things), 2012/08/01 00:26:10]

nit: suggest "minimal range" --> "minimum for a declared range"

same for maximal.

[kaiwang, 2012/08/01 04:13:21]

Done.

+// (HistogramBase::kSampleType_MAX - 1). Currently you can give ranges input

[jar (doing other things), 2012/08/01 00:26:10]

nit: suggest "can give ranges input..." --> 

"can declare histograms with ranges exceeding those limits, but those excesses
will be silently clamped to those limits (for backwards compatibility with
existing code)."

[kaiwang, 2012/08/01 04:13:21]

Done.

+// exceeding this limit, e.g. 0 as minimal or HistogramBase::kSampleType_MAX as
+// maximal. They will be *silently* converted to correct input: 0 and
+// (HistogramBase::kSampleType_MAX - 1). But you should never depend on this!

[jar (doing other things), 2012/08/01 00:26:10]

nit: "But..." --> "Best practice is to not exceed the limits."

[kaiwang, 2012/08/01 04:13:21]

Done.

+
+// For Histogram and LinearHistogram, maximal range should always be larger (not

[jar (doing other things), 2012/08/01 00:26:10]

nit: "maximal range" --> "maximum for a declared range"

same for minimum

[kaiwang, 2012/08/01 04:13:21]

Done.

+// equal) than minmal range. And 0 and HistogramBase::kSampleType_MAX are always

[jar (doing other things), 2012/08/01 00:26:10]

nit: "And 0" --> "Zero"

"are always added" --> "are implicitly added"

[kaiwang, 2012/08/01 04:13:21]

Done.

+// added as first and last ranges, so the minimal bucket count is 3. However

[jar (doing other things), 2012/08/01 00:26:10]

nit: "minimal bucket count" --> "smallest legal bucket_count is 3."

[kaiwang, 2012/08/01 04:13:21]

Done.

+// CustomHistogram can have minimum bucket count as 2 (when you give a custom
+// ranges vector containing only 1 range).
+// For these 3 kind histograms, the max bucket count is always

[jar (doing other things), 2012/08/01 00:26:10]

"kind" --> "kinds of"

[kaiwang, 2012/08/01 04:13:21]

Done.

+// (Histogram::kBucketCount_MAX - 1).
+
+// The buckets layout of Histogram is exponential. For example, buckets might

[jar (doing other things), 2012/08/01 00:26:10]

"Histogram" --> "class Histogram"

[kaiwang, 2012/08/01 04:13:21]

Done.

 // contain (sequentially) the count of values in the following intervals:
 // [0,1), [1,2), [2,4), [4,8), [8,16), [16,32), [32,64), [64,infinity)
 // That bucket allocation would actually result from construction of a histogram
 // for values between 1 and 64, with 8 buckets, such as:
-// Histogram count(L"some name", 1, 64, 8);
+// Histogram count("some name", 1, 64, 8);
 // Note that the underflow bucket [0,1) and the overflow bucket [64,infinity)
-// are not counted by the constructor in the user supplied "bucket_count"
+// are also counted by the constructor in the user supplied "bucket_count"
 // argument.
 // The above example has an exponential ratio of 2 (doubling the bucket width
 // in each consecutive bucket.  The Histogram class automatically calculates
@@ -28,8 +43,9 @@
 // at the low end of the histogram scale, but allows the histogram to cover a
 // gigantic range with the addition of very few buckets.
 
-// Histograms use a pattern involving a function static variable, that is a
-// pointer to a histogram.  This static is explicitly initialized on any thread
+// Usually we use macros to define and use a histogram. These macros use a
+// pattern involving a function static variable, that is a pointer to a
+// histogram.  This static is explicitly initialized on any thread
 // that detects a uninitialized (NULL) pointer.  The potentially racy
 // initialization is not a problem as it is always set to point to the same
 // value (i.e., the FactoryGet always returns the same value).  FactoryGet
@@ -177,9 +193,10 @@
             boundary_value + 1, base::Histogram::kNoFlags))
 
 // Support histograming of an enumerated value. Samples should be one of the
-// std::vector<int> list provided via |custom_ranges|. You can use the helper
-// function |base::CustomHistogram::ArrayToCustomRanges(samples, num_samples)|
-// to transform a C-style array of valid sample values to a std::vector<int>.
+// std::vector<int> list provided via |custom_ranges|. See comments above
+// CustomRanges::FactoryGet about the requirement of |custom_ranges|.
+// You can use the helper function CustomHistogram::ArrayToCustomRanges to
+// transform a C-style array of valid sample values to a std::vector<int>.
 #define HISTOGRAM_CUSTOM_ENUMERATION(name, sample, custom_ranges) \
     STATIC_HISTOGRAM_POINTER_BLOCK(name, Add(sample), \
         base::CustomHistogram::FactoryGet(name, custom_ranges, \
@@ -338,13 +355,13 @@
     LINEAR_HISTOGRAM,
     BOOLEAN_HISTOGRAM,
     CUSTOM_HISTOGRAM,
-    NOT_VALID_IN_RENDERER
+    NOT_VALID_IN_RENDERER,
   };
 
   enum BucketLayout {
     EXPONENTIAL,
     LINEAR,
-    CUSTOM
+    CUSTOM,
   };
 
   enum Flags {
@@ -381,17 +398,17 @@
 
   class BASE_EXPORT SampleSet {
    public:
-    explicit SampleSet();
+    explicit SampleSet(size_t size);
+    SampleSet();
     ~SampleSet();
 
-    // Adjust size of counts_ for use with given histogram.
-    void Resize(const Histogram& histogram);
-    void CheckSize(const Histogram& histogram) const;
+    void Resize(size_t size);
 
     // Accessor for histogram to make routine additions.
     void Accumulate(Sample value, Count count, size_t index);
 
     // Accessor methods.
+    size_t size() const { return counts_.size(); }
     Count counts(size_t i) const { return counts_[i]; }
     Count TotalCount() const;
     int64 sum() const { return sum_; }
@@ -447,10 +464,32 @@
                                    base::TimeDelta maximum,
                                    size_t bucket_count,
                                    Flags flags);
+
   // Time call for use with DHISTOGRAM*.
   // Returns TimeTicks::Now() in debug and TimeTicks() in release build.
   static TimeTicks DebugNow();
 
+  // Convenience methods for serializing/deserializing the histograms.
+  // Histograms from Renderer process are serialized and sent to the browser.
+  // Browser process reconstructs the histogram from the pickled version
+  // accumulates the browser-side shadow copy of histograms (that mirror
+  // histograms created in the renderer).
+
+  // Serialize the given snapshot of a Histogram into a String. Uses
+  // Pickle class to flatten the object.
+  static std::string SerializeHistogramInfo(const Histogram& histogram,

[jar (doing other things), 2012/08/01 00:26:10]

note that grouping by "static" is not in style guide... so the old order was
perchance based on utility.

Perhaps it should remain where it was (until you do a more major rewrite).

[kaiwang, 2012/08/01 04:13:21]

Done.

+                                            const SampleSet& snapshot);
+
+  // The following method accepts a list of pickled histograms and
+  // builds a histogram and updates shadow copy of histogram data in the
+  // browser process.
+  static bool DeserializeHistogramInfo(const std::string& histogram_info);
+
+  static void InitializeBucketRanges(Sample minimum,
+                                     Sample maximum,
+                                     size_t bucket_count,
+                                     BucketRanges* ranges);
+
   virtual void Add(Sample value) OVERRIDE;
 
   // This method is an interface, used only by BooleanHistogram.
@@ -477,22 +516,6 @@
   void ClearFlags(Flags flags) { flags_ = static_cast<Flags>(flags_ & ~flags); }
   int flags() const { return flags_; }
 
-  // Convenience methods for serializing/deserializing the histograms.
-  // Histograms from Renderer process are serialized and sent to the browser.
-  // Browser process reconstructs the histogram from the pickled version
-  // accumulates the browser-side shadow copy of histograms (that mirror
-  // histograms created in the renderer).
-
-  // Serialize the given snapshot of a Histogram into a String. Uses
-  // Pickle class to flatten the object.
-  static std::string SerializeHistogramInfo(const Histogram& histogram,
-                                            const SampleSet& snapshot);
-
-  // The following method accepts a list of pickled histograms and
-  // builds a histogram and updates shadow copy of histogram data in the
-  // browser process.
-  static bool DeserializeHistogramInfo(const std::string& histogram_info);
-
   // Check to see if bucket ranges, counts and tallies in the snapshot are
   // consistent with the bucket ranges and checksums in our histogram.  This can
   // produce a false-alarm if a race occurred in the reading of the data during
@@ -507,33 +530,35 @@
   Sample declared_min() const { return declared_min_; }
   Sample declared_max() const { return declared_max_; }
   virtual Sample ranges(size_t i) const;
-  uint32 range_checksum() const { return range_checksum_; }
   virtual size_t bucket_count() const;
-  BucketRanges* bucket_ranges() const { return bucket_ranges_; }
-  void set_bucket_ranges(BucketRanges* bucket_ranges) {
-    bucket_ranges_ = bucket_ranges;
-  }
+  const BucketRanges* bucket_ranges() const { return bucket_ranges_; }
+
   // Snapshot the current complete set of sample data.
   // Override with atomic/locked snapshot if needed.
   virtual void SnapshotSample(SampleSet* sample) const;
 
-  virtual bool HasConstructorArguments(Sample minimum, Sample maximum,
-                                       size_t bucket_count);
+  virtual bool HasConstructionArguments(Sample minimum,
+                                        Sample maximum,
+                                        size_t bucket_count);
+  virtual ~Histogram();

[jar (doing other things), 2012/08/01 00:26:10]

nit: move to near top of public section.

Perhaps this should stay as protected... and eventually be private when we stop
deriving from this class.

For now, the derived class destructors should probably be protected, or private.

We want to strongly discourage deletion... and at most, only allow such in test
code (which may FRIEND to facilitate).

[kaiwang, 2012/08/01 04:13:21]

Done.

 
-  virtual bool HasConstructorTimeDeltaArguments(TimeDelta minimum,
-                                                TimeDelta maximum,
-                                                size_t bucket_count);
-  // Return true iff the range_checksum_ matches current |ranges_| vector in
-  // |bucket_ranges_|.
-  bool HasValidRangeChecksum() const;
-
  protected:
-  Histogram(const std::string& name, Sample minimum,
-            Sample maximum, size_t bucket_count);
-  Histogram(const std::string& name, TimeDelta minimum,
-            TimeDelta maximum, size_t bucket_count);
+  // This function validates histogram construction arguments. It returns false
+  // if some of the arguments are totally bad.
+  // Note. Currently it allow some bad input, e.g. 0 as minimum, but silently
+  // converts it to good input: 1.
+  // TODO(kaiwang): Be more restrict and return false for any bad input, and
+  // make this a readonly validating function.
+  static bool InspectConstructionArguments(const std::string& name,
+                                           Sample* minimum,
+                                           Sample* maximum,
+                                           size_t* bucket_count);
 
-  virtual ~Histogram();
+  Histogram(const std::string& name,

[jar (doing other things), 2012/08/01 00:26:10]

nit: push to top of section.

[kaiwang, 2012/08/01 04:13:21]

Done.

+            Sample minimum,
+            Sample maximum,
+            size_t bucket_count,
+            const BucketRanges* ranges);
 
   // Serialize the histogram's ranges to |*pickle|, returning true on success.
   // Most subclasses can leave this no-op implementation, but some will want to
@@ -541,9 +566,6 @@
   // serialized parameters.
   virtual bool SerializeRanges(Pickle* pickle) const;
 
-  // Initialize ranges_ mapping in bucket_ranges_.
-  void InitializeBucketRange();
-
   // Method to override to skip the display of the i'th bucket if it's empty.
   virtual bool PrintEmptyBucket(size_t index) const;
 
@@ -555,9 +577,6 @@
   // Get normalized size, relative to the ranges(i).
   virtual double GetBucketSize(Count current, size_t i) const;
 
-  // Recalculate range_checksum_.
-  void ResetRangeChecksum();
-
   // Return a string description of what goes in a given bucket.
   // Most commonly this is the numeric value, but in derived classes it may
   // be a name (or string description) given to the bucket.
@@ -569,18 +588,6 @@
   // Update all our internal data, including histogram
   virtual void Accumulate(Sample value, Count count, size_t index);
 
-  //----------------------------------------------------------------------------
-  // Accessors for derived classes.
-  //----------------------------------------------------------------------------
-  void SetBucketRange(size_t i, Sample value);
-
-  // Validate that ranges_ in bucket_ranges_ was created sensibly (top and
-  // bottom range values relate properly to the declared_min_ and
-  // declared_max_).
-  bool ValidateBucketRanges() const;
-
-  virtual uint32 CalculateRangeChecksum() const;
-
  private:
   // Allow tests to corrupt our innards for testing purposes.
   FRIEND_TEST_ALL_PREFIXES(HistogramTest, CorruptBucketBounds);
@@ -590,12 +597,6 @@
 
   friend class StatisticsRecorder;  // To allow it to delete duplicates.
 
-  // Post constructor initialization.
-  void Initialize();
-
-  // Checksum function for accumulating range values into a checksum.
-  static uint32 Crc32(uint32 sum, Sample range);
-
   //----------------------------------------------------------------------------
   // Helpers for emitting Ascii graphic.  Each method appends data to output.
 
@@ -625,11 +626,8 @@
   void WriteAsciiBucketGraph(double current_size, double max_size,
                              std::string* output) const;
 
-  //----------------------------------------------------------------------------
-  // Table for generating Crc32 values.
-  static const uint32 kCrcTable[256];
-  //----------------------------------------------------------------------------
-  // Invariant values set at/near construction time
+  // Does not own this object. Should get from StatisticsRecorder.
+  const BucketRanges* bucket_ranges_;
 
   Sample declared_min_;  // Less than this goes into counts_[0]
   Sample declared_max_;  // Over this goes into counts_[bucket_count_ - 1].
@@ -638,17 +636,6 @@
   // Flag the histogram for recording by UMA via metric_services.h.
   Flags flags_;
 
-  // For each index, show the least value that can be stored in the
-  // corresponding bucket. We also append one extra element in this array,
-  // containing kSampleType_MAX, to make calculations easy.
-  // The dimension of ranges_ in bucket_ranges_ is bucket_count + 1.

[jar (doing other things), 2012/08/01 00:26:10]

nit: Preserve this comment, since we don't explain (elsewhere)

... or figure out where to place it.

I think it is helpful to describe a slot.... but perhaps you wanted to try to
describe it at the "setter" which is in the constructor.... I'm not sure. 

[kaiwang, 2012/08/01 04:13:21]

Done. Simplified the comments and added to Histogram constructor

-  BucketRanges* bucket_ranges_;
-
-  // For redundancy, we store a checksum of all the sample ranges when ranges
-  // are generated.  If ever there is ever a difference, then the histogram must
-  // have been corrupted.
-  uint32 range_checksum_;
-
   // Finally, provide the state that changes with the addition of each new
   // sample.
   SampleSet sample_;
@@ -677,6 +664,11 @@
                                    size_t bucket_count,
                                    Flags flags);
 
+  static void InitializeBucketRanges(Sample minimum,
+                                     Sample maximum,
+                                     size_t bucket_count,
+                                     BucketRanges* ranges);
+
   // Overridden from Histogram:
   virtual ClassType histogram_type() const OVERRIDE;
 
@@ -686,14 +678,12 @@
       const DescriptionPair descriptions[]) OVERRIDE;
 
  protected:
-  LinearHistogram(const std::string& name, Sample minimum,
-                  Sample maximum, size_t bucket_count);
+  LinearHistogram(const std::string& name,
+                  Sample minimum,
+                  Sample maximum,
+                  size_t bucket_count,
+                  const BucketRanges* ranges);
 
-  LinearHistogram(const std::string& name, TimeDelta minimum,
-                  TimeDelta maximum, size_t bucket_count);
-
-  // Initialize ranges_ mapping in bucket_ranges_.
-  void InitializeBucketRange();
   virtual double GetBucketSize(Count current, size_t i) const OVERRIDE;
 
   // If we have a description for a bucket, then return that.  Otherwise
@@ -726,7 +716,7 @@
   virtual void AddBoolean(bool value) OVERRIDE;
 
  private:
-  explicit BooleanHistogram(const std::string& name);
+  BooleanHistogram(const std::string& name, const BucketRanges* ranges);
 
   DISALLOW_COPY_AND_ASSIGN(BooleanHistogram);
 };
@@ -736,7 +726,10 @@
 // CustomHistogram is a histogram for a set of custom integers.
 class BASE_EXPORT CustomHistogram : public Histogram {
  public:
-
+  // |custom_ranges| contains a set of ranges. Each of them should be > 0 and

[jar (doing other things), 2012/08/01 00:26:10]

nit: "a set of ranges" --> "a vector of limits on ranges".

"Each of them" --> "Each limit"

[kaiwang, 2012/08/01 04:13:21]

Done.

+  // < kSampleType_MAX. (Currently 0 is still accepted for backward
+  // compatibility). The ranges can be unordered or contain duplication, but

[jar (doing other things), 2012/08/01 00:26:10]

nit: "ranges" --> "limits"

[kaiwang, 2012/08/01 04:13:21]

Done.

+  // client should not depend on this.
   static Histogram* FactoryGet(const std::string& name,
                                const std::vector<Sample>& custom_ranges,
                                Flags flags);
@@ -755,19 +748,22 @@
   // Helper for deserializing CustomHistograms.  |*ranges| should already be
   // correctly sized before this call.  Return true on success.
   static bool DeserializeRanges(PickleIterator* iter,
-                                std::vector<Histogram::Sample>* ranges);
+                                std::vector<Sample>* ranges);
 
 
  protected:
   CustomHistogram(const std::string& name,
-                  const std::vector<Sample>& custom_ranges);
+                  const BucketRanges* ranges);
 
   virtual bool SerializeRanges(Pickle* pickle) const OVERRIDE;
 
-  // Initialize ranges_ mapping in bucket_ranges_.
-  void InitializedCustomBucketRange(const std::vector<Sample>& custom_ranges);
   virtual double GetBucketSize(Count current, size_t i) const OVERRIDE;
 
+ private:
+  static bool ValidateCustomRanges(const std::vector<Sample>& custom_ranges);
+  static BucketRanges* CreateBucketRangesFromCustomRanges(
+      const std::vector<Sample>& custom_ranges);
+
   DISALLOW_COPY_AND_ASSIGN(CustomHistogram);
 };