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,61 @@
 class Histogram;
 class LinearHistogram;
 
+class BASE_EXPORT_PRIVATE BucketHistogramSamples : public HistogramSamples {

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

I don't really understand what a "BucketHistogram" is -- don't all histograms
have buckets?

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

SparseHistograms don't

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

SparseHistograms don't have *contiguous* buckets, but surely they still have
buckets.  A histogram bucket is essentially a key/value pair.  Am I missing
something?

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

(bump)

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

Original Histogram separates the whole sample value space into different parts,
and we call these different parts "buckets". By this definition, sparse
histograms do not have predefined buckets, they only have precise values.

Based on this difference, I suggested to Jim to call Histogram
"BucketHistogram", comparing to "SparseHistogram". But it's hard to change the
existing class name now.

[Ilya Sherman, 2012/08/30 07:57:39]

I think we disagree on what a histogram "bucket" means.  Unless I'm mistaken,
most people would describe a SparseHistogram as also having buckets -- just
sparse ones.  A bucket (or, as the Wikipedia page calls them [1], a "bin"), is
simply the interval to which a set of samples correspond.  Sparse histograms
still group samples into bins/buckets.

How about ContinuousHistogramSamples, to distinguish from (presumably)
SparseHistogramSamples?

[1] http://en.wikipedia.org/wiki/Histogram

[jar (doing other things), 2012/08/30 22:54:38]

I think perchance you meant ContiGuousHistogramSamples instead of
ContinuousHistogramSamples... since you used that word... but both are pretty
long :-(.

How about using SampleVector here, and then using SampleMap for sparse
histograms??  This seemed plausible when I chatted with Kai.

Ilya: WDYT?

On 2012/08/30 07:57:39, Ilya Sherman wrote:

[Ilya Sherman, 2012/08/30 23:01:46]

SGTM

+ public:
+  BucketHistogramSamples(const BucketRanges* bucket_ranges);

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

nit: explicit

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

nit: Can this be passed by const-ref rather than const-pointer?

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

Done.

+  virtual ~BucketHistogramSamples();
+
+  virtual void Accumulate(HistogramBase::Sample value,

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

nit: Please label these methods with something like "// HistogramSamples
implementation."

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

Done.

+                          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;

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

nit: "From" -> "For"

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

I really don't think "For" is better than "From", but if you like, I can change
it to "GetCountWithBucketIndex", which may sound better to me.

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

If you want to use "from", the name should be "GetCountFromBucketWithIndex()" --
the bucket *index* does not have a count.  The count also does not have a bucket
index, so the alternate name you suggested doesn't quite work either.

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

What do you think of GetCountAtIndex? from Jim's suggestion

[Ilya Sherman, 2012/08/30 07:57:39]

Works for me :)

+
+ protected:
+  virtual bool AddSubtractHelper(SampleCountIterator* iter,
+                                 bool is_add) OVERRIDE;
+
+  static size_t BucketIndex(HistogramBase::Sample value,

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

nit: GetBucketIndex()

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

Done.

+                            const BucketRanges* bucket_ranges);

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

nit: Can this be passed by const-reference?

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BucketHistogramSamples);

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

Optional nit: I think this is usually the very last line in a class.

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

Done.

+  FRIEND_TEST_ALL_PREFIXES(HistogramTest, CorruptSampleCounts);
+
+  std::vector<HistogramBase::Count> counts_;
+
+  // Shares the same BucketRanges with Histogram object.
+  const BucketRanges* bucket_ranges_;

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

nit: Can this be a const reference or a const BucketRanges* const?

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

Done.

+};
+
+class BASE_EXPORT_PRIVATE BucketHistogramSamplesIterator
+    : public SampleCountIterator {
+ public:
+  BucketHistogramSamplesIterator(
+      const std::vector<HistogramBase::Count>* counts,

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

nit: Can this be passed by const-reference?

+      const BucketRanges* bucket_ranges);

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

nit: Ditto

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

For all the reference related comments:
IMHO, I rarely see reference class members in google code base. I feel it's
better to use pointers here, because the objects are owned by others and stored
in other class as pointers (so the pointer here is weak). But in C++, reference
is just a "name", and does not have the "weak" semantic. So in this case it may
cause confusions.
Passing in const pointers are perfectly valid according to style guide.

+
+  virtual bool Done() OVERRIDE;
+  virtual void Next() OVERRIDE;
+  virtual void Get(HistogramBase::Sample* min,
+                   HistogramBase::Sample* max,
+                   HistogramBase::Count* count) OVERRIDE;
+ private:
+  void ForwardIndex();  // Find the next place with data.

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

nit: Please choose a more descriptive name rather than relying on the comment.

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

Done.

+
+  const std::vector<HistogramBase::Count>* counts_;
+  const BucketRanges* bucket_ranges_;
+
+  int index_;
+  bool is_done_;
+};

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

Please declare & define these classes in separate files.

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

This file currently contains all stuff related to Histogram (5 classes and
macros). Only moving samples related code out does not help to clarify.

I agree the best way is to move every class to different file. But it needs a
lot of code change. I'd prefer to do it in seperate CLs

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

You are adding a new class, so moving the new class out does not add extra code.
 Please move this code out in this CL.  I agree that splitting up this header
further does not make sense as part of this CL.

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

(bump)

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

Done.

+
 class BASE_EXPORT Histogram : public HistogramBase {
  public:
   // Initialize maximum number of buckets in histograms as 16,384.
@@ -473,7 +530,8 @@
     Add(static_cast<int>(time.InMilliseconds()));
   }
 
-  void AddSampleSet(const SampleSet& sample);
+  void AddSamples(const HistogramSamples& samples);
+  bool AddSamples(PickleIterator* iter);
 
   // This method is an interface, used only by LinearHistogram.
   virtual void SetRangeDescriptions(const DescriptionPair descriptions[]);
@@ -491,19 +549,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

[jar (doing other things), 2012/08/30 22:54:38]

nit: out --> our

[kaiwang, 2012/09/07 01:14:11]

Done.

+  // 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 +584,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 +620,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 +628,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 +645,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 +677,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);
 };