SampleSet -> HistogramSamples (will be reused by SparseHistogram)

base/metrics/histogram.h

Index: base/metrics/histogram.h
===================================================================
--- base/metrics/histogram.h	(revision 152603)
+++ base/metrics/histogram.h	(working copy)
@@ -68,8 +68,10 @@
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/metrics/bucket_ranges.h"
 #include "base/metrics/histogram_base.h"
+#include "base/metrics/histogram_samples.h"
 #include "base/time.h"
 
 class Pickle;
@@ -344,6 +346,63 @@
 class Histogram;
 class LinearHistogram;
 
+class BASE_EXPORT_PRIVATE BucketHistogramSamples : public HistogramSamples {
+ public:
+  explicit BucketHistogramSamples(const BucketRanges* bucket_ranges);
+  virtual ~BucketHistogramSamples();
+
+  // Implements HistogramSamples:

[Ilya Sherman, 2012/08/29 08:48:16]

nit: This is typically written as "HistogramSamples implementation:" or just
"HistogramSamples:"

[kaiwang, 2012/08/29 22:42:16]

Done.

+  virtual void Accumulate(HistogramBase::Sample value,
+                          HistogramBase::Count count) OVERRIDE;
+  virtual HistogramBase::Count GetCount(
+      HistogramBase::Sample value) const OVERRIDE;
+  virtual HistogramBase::Count TotalCount() const OVERRIDE;
+  virtual scoped_ptr<SampleCountIterator> Iterator() const OVERRIDE;
+
+  // Get count of a specific bucket.
+  HistogramBase::Count GetCountFromBucketIndex(size_t bucket_index) const;
+
+ protected:
+  virtual bool AddSubtractImpl(SampleCountIterator* iter,
+                               bool is_add) OVERRIDE;
+
+  static size_t GetBucketIndex(HistogramBase::Sample value,
+                               const BucketRanges* bucket_ranges);

[Ilya Sherman, 2012/08/29 08:48:16]

nit: Why is this a static function rather than an instance method?  It looks
like you always pass |bucket_ranges_| as the second parameter.

[Ilya Sherman, 2012/08/29 08:48:16]

Can this be private?

[kaiwang, 2012/08/29 22:42:16]

Will change it to private.

For static/non-static, there's a coding best practice(inside and outside of
google) to define private functions that does not change object state all
static. Unfortunately I can not find the internal pages at hand.

The good thing about static is, it does not depends on the whole object (no this
pointer). So it's much easier for code reader to understand what is going on if
you pass the
members that affect this method. So basically static function has less
dependency then non-static functions.

Another reason is, making private functions static does not help encapsulation,
because external client can not call these functions. While many public
functions has to be non-static, because external client need them to change
object state.

I do find some design docs of another project mentioning this. It's the maps
frontend testing design doc:
https://www.corp.google.com/eng/designdocs/maps/testing.html#s13

[Ilya Sherman, 2012/08/29 23:41:27]

As discussed offline, Chromium code usually prefers free functions hidden in the
implementation file's anonymous namespace to private static functions.

Also, please keep in mind that this code review site is used by more than just
Googlers, so it is generally not appropriate to link to internal-only pages,
including documentation.  As much as possible, all relevant resources should be
made equally accessible to all Chromium developers.  (In this case it's not such
a big issue, since you explained the contents fairly clearly, and they are not
of a sensitive nature.  Just something to keep in mind.)

[kaiwang, 2012/08/30 03:13:22]

Sorry I forgot a reason to make it a non-static member: subclass can override it
in a more efficient way. E.g. LinearHistogram should be able to directly
calculate the index.
Changed it back to non-static.

+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(HistogramTest, CorruptSampleCounts);
+
+  std::vector<HistogramBase::Count> counts_;
+
+  // Shares the same BucketRanges with Histogram object.
+  const BucketRanges* const bucket_ranges_;
+
+  DISALLOW_COPY_AND_ASSIGN(BucketHistogramSamples);
+};
+
+class BASE_EXPORT_PRIVATE BucketHistogramSamplesIterator
+    : public SampleCountIterator {
+ public:
+  BucketHistogramSamplesIterator(
+      const std::vector<HistogramBase::Count>* counts,
+      const BucketRanges* bucket_ranges);
+
+  // Implements SampleCountIterator.
+  virtual bool Done() OVERRIDE;
+  virtual void Next() OVERRIDE;
+  virtual void Get(HistogramBase::Sample* min,
+                   HistogramBase::Sample* max,
+                   HistogramBase::Count* count) OVERRIDE;
+ private:
+  void ForwardToNextNonemptyBucket();
+
+  const std::vector<HistogramBase::Count>* counts_;
+  const BucketRanges* bucket_ranges_;
+
+  int index_;
+  bool is_done_;
+};
+
 class BASE_EXPORT Histogram : public HistogramBase {
  public:
   // Initialize maximum number of buckets in histograms as 16,384.
@@ -383,57 +442,6 @@
   };
 
   //----------------------------------------------------------------------------
-  // Statistic values, developed over the life of the histogram.
-
-  class BASE_EXPORT SampleSet {
-   public:
-    explicit SampleSet(size_t size);
-    SampleSet();
-    ~SampleSet();
-
-    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_; }
-    int64 redundant_count() const { return redundant_count_; }
-
-    // Arithmetic manipulation of corresponding elements of the set.
-    void Add(const SampleSet& other);
-    void Subtract(const SampleSet& other);
-
-    bool Serialize(Pickle* pickle) const;
-    bool Deserialize(PickleIterator* iter);
-
-   protected:
-    // Actual histogram data is stored in buckets, showing the count of values
-    // that fit into each bucket.
-    Counts counts_;
-
-    // Save simple stats locally.  Note that this MIGHT get done in base class
-    // without shared memory at some point.
-    int64 sum_;         // sum of samples.
-
-   private:
-    // Allow tests to corrupt our innards for testing purposes.
-    FRIEND_TEST_ALL_PREFIXES(HistogramTest, CorruptSampleCounts);
-
-    // To help identify memory corruption, we reduntantly save the number of
-    // samples we've accumulated into all of our buckets.  We can compare this
-    // count to the sum of the counts in all buckets, and detect problems.  Note
-    // that due to races in histogram accumulation (if a histogram is indeed
-    // updated on several threads simultaneously), the tallies might mismatch,
-    // and also the snapshotting code may asynchronously get a mismatch (though
-    // generally either race based mismatch cause is VERY rare).
-    int64 redundant_count_;
-  };
-
-  //----------------------------------------------------------------------------
   // For a valid histogram, input should follow these restrictions:
   // minimum > 0 (if a minimum below 1 is specified, it will implicitly be
   //              normalized up to 1)
@@ -473,7 +481,8 @@
     Add(static_cast<int>(time.InMilliseconds()));
   }
 
-  void AddSampleSet(const SampleSet& sample);
+  void AddSamples(const HistogramSamples& samples);
+  bool AddSamples(PickleIterator* iter);

[Ilya Sherman, 2012/08/29 08:48:16]

Optional nit: Perhaps |AddSamplesFromPickle()|?

[kaiwang, 2012/08/29 22:42:16]

Done.

 
   // This method is an interface, used only by LinearHistogram.
   virtual void SetRangeDescriptions(const DescriptionPair descriptions[]);
@@ -491,19 +500,28 @@
   // 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);
+                                            const HistogramSamples& 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);
 
+  // This constant if for FindCorruption. Since snapshots of histograms are
+  // taken asynchronously relative to sampling, and out counting code currently
+  // does not prevent race conditions, it is pretty likely that we'll catch a
+  // redundant count that doesn't match the sample count.  We allow for a
+  // certain amount of slop before flagging this as an inconsistency. Even with
+  // an inconsistency, we'll snapshot it again (for UMA in about a half hour),
+  // so we'll eventually get the data, if it was not the result of a corruption.
+  static const int kCommonRaceBasedCountMismatch;
+
   // 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
   // a SnapShot process, but should otherwise be false at all times (unless we
   // have memory over-writes, or DRAM failures).
-  virtual Inconsistencies FindCorruption(const SampleSet& snapshot) const;
+  virtual Inconsistencies FindCorruption(const HistogramSamples& samples) const;
 
   //----------------------------------------------------------------------------
   // Accessors for factory constuction, serialization and testing.
@@ -517,7 +535,7 @@
 
   // Snapshot the current complete set of sample data.
   // Override with atomic/locked snapshot if needed.
-  virtual void SnapshotSample(SampleSet* sample) const;
+  virtual scoped_ptr<BucketHistogramSamples> SnapshotSamples() const;
 
   virtual bool HasConstructionArguments(Sample minimum,
                                         Sample maximum,
@@ -553,11 +571,6 @@
   // Method to override to skip the display of the i'th bucket if it's empty.
   virtual bool PrintEmptyBucket(size_t index) const;
 
-  //----------------------------------------------------------------------------
-  // Methods to override to create histogram with different bucket widths.
-  //----------------------------------------------------------------------------
-  // Find bucket to increment for sample value.
-  virtual size_t BucketIndex(Sample value) const;
   // Get normalized size, relative to the ranges(i).
   virtual double GetBucketSize(Count current, size_t i) const;
 
@@ -566,12 +579,6 @@
   // be a name (or string description) given to the bucket.
   virtual const std::string GetAsciiBucketRange(size_t it) const;
 
-  //----------------------------------------------------------------------------
-  // Methods to override to create thread safe histogram.
-  //----------------------------------------------------------------------------
-  // Update all our internal data, including histogram
-  virtual void Accumulate(Sample value, Count count, size_t index);
-
  private:
   // Allow tests to corrupt our innards for testing purposes.
   FRIEND_TEST_ALL_PREFIXES(HistogramTest, CorruptBucketBounds);
@@ -589,12 +596,13 @@
                       const std::string& newline,
                       std::string* output) const;
 
-  // Find out how large the (graphically) the largest bucket will appear to be.
-  double GetPeakBucketSize(const SampleSet& snapshot) const;
+  // Find out how large (graphically) the largest bucket will appear to be.
+  double GetPeakBucketSize(const BucketHistogramSamples& samples) const;
 
   // Write a common header message describing this histogram.
-  void WriteAsciiHeader(const SampleSet& snapshot,
-                        Count sample_count, std::string* output) const;
+  void WriteAsciiHeader(const BucketHistogramSamples& samples,
+                        Count sample_count,
+                        std::string* output) const;
 
   // Write information about previous, current, and next buckets.
   // Information such as cumulative percentage, etc.
@@ -620,7 +628,7 @@
 
   // Finally, provide the state that changes with the addition of each new
   // sample.
-  SampleSet sample_;
+  scoped_ptr<BucketHistogramSamples> samples_;
 
   DISALLOW_COPY_AND_ASSIGN(Histogram);
 };