SampleSet -> HistogramSamples (will be reused by SparseHistogram)

base/metrics/histogram.h

 // 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).
 
 // It supports calls to accumulate either time intervals (which are processed
 // as integral number of milliseconds), or arbitrary integral units.
@@ skipping 50 matching lines @@
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/atomicops.h"
 #include "base/base_export.h"
 #include "base/basictypes.h"
 #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;
 class PickleIterator;
 
 namespace base {
 
 class Lock;
 //------------------------------------------------------------------------------
 // Histograms are often put in areas where they are called many many times, and
@@ skipping 254 matching lines @@
             base::Histogram::kUmaTargetedHistogramFlag))
 
 //------------------------------------------------------------------------------
 
 class BooleanHistogram;
 class BucketRanges;
 class CustomHistogram;
 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.
   static const size_t kBucketCount_MAX;
 
   typedef std::vector<Count> Counts;
 
   // These enums are used to facilitate deserialization of renderer histograms
   // into the browser.
   enum ClassType {
@@ skipping 109 matching lines @@
   virtual void Add(Sample value) OVERRIDE;
 
   // This method is an interface, used only by BooleanHistogram.
   virtual void AddBoolean(bool value);
 
   // Accept a TimeDelta to increment.
   void AddTime(TimeDelta time) {
     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[]);
 
   // The following methods provide graphical histogram displays.
   virtual void WriteHTMLGraph(std::string* output) const OVERRIDE;
   virtual void WriteAscii(std::string* output) const OVERRIDE;
 
   // 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);
+                                            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.
   //----------------------------------------------------------------------------
   virtual ClassType histogram_type() const;
   Sample declared_min() const { return declared_min_; }
   Sample declared_max() const { return declared_max_; }
   virtual Sample ranges(size_t i) const;
   virtual size_t bucket_count() const;
   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 scoped_ptr<BucketHistogramSamples> SnapshotSamples() const;
 
   virtual bool HasConstructionArguments(Sample minimum,
                                         Sample maximum,
                                         size_t bucket_count);
  protected:
   // |bucket_count| and |ranges| should contain the underflow and overflow
   // buckets. See top comments for example.
   Histogram(const std::string& name,
             Sample minimum,
             Sample maximum,
@@ skipping 15 matching lines @@
 
   // Serialize the histogram's ranges to |*pickle|, returning true on success.
   // Most subclasses can leave this no-op implementation, but some will want to
   // override it, especially if the ranges cannot be re-derived from other
   // serialized parameters.
   virtual bool SerializeRanges(Pickle* pickle) const;
 
   // 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;
 
   // 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.
   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);
   FRIEND_TEST_ALL_PREFIXES(HistogramTest, CorruptSampleCounts);
   FRIEND_TEST_ALL_PREFIXES(HistogramTest, Crc32SampleHash);
   FRIEND_TEST_ALL_PREFIXES(HistogramTest, Crc32TableTest);
 
   friend class StatisticsRecorder;  // To allow it to delete duplicates.
   friend class StatisticsRecorderTest;
 
   //----------------------------------------------------------------------------
   // Helpers for emitting Ascii graphic.  Each method appends data to output.
 
   void WriteAsciiImpl(bool graph_it,
                       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.
   void WriteAsciiBucketContext(const int64 past, const Count current,
                                const int64 remaining, const size_t i,
                                std::string* output) const;
 
   // Write textual description of the bucket contents (relative to histogram).
   // Output is the count in the buckets, as well as the percentage.
   void WriteAsciiBucketValue(Count current, double scaled_sum,
                              std::string* output) const;
 
   // Produce actual graph (set of blank vs non blank char's) for a bucket.
   void WriteAsciiBucketGraph(double current_size, double max_size,
                              std::string* output) const;
 
   // 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].
   size_t bucket_count_;  // Dimension of counts_[].
 
   // Finally, provide the state that changes with the addition of each new
   // sample.
-  SampleSet sample_;
+  scoped_ptr<BucketHistogramSamples> samples_;
 
   DISALLOW_COPY_AND_ASSIGN(Histogram);
 };
 
 //------------------------------------------------------------------------------
 
 // LinearHistogram is a more traditional histogram, with evenly spaced
 // buckets.
 class BASE_EXPORT LinearHistogram : public Histogram {
  public:
@@ skipping 110 matching lines @@
   static bool ValidateCustomRanges(const std::vector<Sample>& custom_ranges);
   static BucketRanges* CreateBucketRangesFromCustomRanges(
       const std::vector<Sample>& custom_ranges);
 
   DISALLOW_COPY_AND_ASSIGN(CustomHistogram);
 };
 
 }  // namespace base
 
 #endif  // BASE_METRICS_HISTOGRAM_H_