Add SampleMap and use it in SparseHistogram

base/metrics/sample_map.h

Index: base/metrics/sample_map.h
diff --git a/base/metrics/sample_vector.h b/base/metrics/sample_map.h
similarity index 51%
copy from base/metrics/sample_vector.h
copy to base/metrics/sample_map.h
index 8938d362a754b99626b7110d7b6dcc540f3913bb..7419e528d57472f10ad258d19695849d83f5899a 100644
--- a/base/metrics/sample_vector.h
+++ b/base/metrics/sample_map.h
@@ -3,27 +3,25 @@
 // found in the LICENSE file.
 
 // SampleVector implements HistogramSamples interface. It is used by all

[Ilya Sherman, 2012/10/01 21:24:50]

nit: SampleMap

[kaiwang, 2012/10/01 22:38:52]

Done.

-// Histogram based classes to store samples.
+// SparseHistogram to store samples.

[Ilya Sherman, 2012/10/01 21:24:50]

Optional nit: For the second sentence, perhaps something like "It is used by the
SparseHistogram class to store samples." ?

[kaiwang, 2012/10/01 22:38:52]

Done.

 
-#ifndef BASE_METRICS_SAMPLE_VECTOR_H_
-#define BASE_METRICS_SAMPLE_VECTOR_H_
+#ifndef BASE_METRICS_SAMPLE_MAP_H_
+#define BASE_METRICS_SAMPLE_MAP_H_
 
-#include <vector>
+#include <map>
 
 #include "base/compiler_specific.h"
-#include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram_base.h"
 #include "base/metrics/histogram_samples.h"
+#include "base/synchronization/lock.h"
 
 namespace base {
 
-class BucketRanges;
-
-class BASE_EXPORT_PRIVATE SampleVector : public HistogramSamples {
+class BASE_EXPORT_PRIVATE SampleMap : public HistogramSamples {
  public:
-  explicit SampleVector(const BucketRanges* bucket_ranges);
-  virtual ~SampleVector();
+  SampleMap();
+  virtual ~SampleMap();
 
   // HistogramSamples implementation:
   virtual void Accumulate(HistogramBase::Sample value,
@@ -33,31 +31,27 @@ class BASE_EXPORT_PRIVATE SampleVector : public HistogramSamples {
   virtual HistogramBase::Count TotalCount() const OVERRIDE;
   virtual scoped_ptr<SampleCountIterator> Iterator() const OVERRIDE;
 
-  // Get count of a specific bucket.
-  HistogramBase::Count GetCountAtIndex(size_t bucket_index) const;
-
  protected:
   virtual bool AddSubtractImpl(
       SampleCountIterator* iter,
       HistogramSamples::Instruction instruction) OVERRIDE;
 
-  virtual size_t GetBucketIndex(HistogramBase::Sample value) const;
-
  private:
-  FRIEND_TEST_ALL_PREFIXES(HistogramTest, CorruptSampleCounts);
-
-  std::vector<HistogramBase::Count> counts_;
+  std::map<HistogramBase::Sample, HistogramBase::Count> sample_count_;

[Ilya Sherman, 2012/10/01 21:24:50]

nit: I think either |sample_counts_| or just |counts_| would be a slightly
better name, since this is a map containing multiple counts.

[kaiwang, 2012/10/01 22:38:52]

Done.

 
-  // Shares the same BucketRanges with Histogram object.
-  const BucketRanges* const bucket_ranges_;
+  // Protects access to above map.
+  mutable base::Lock lock_;

[Ilya Sherman, 2012/10/01 21:24:50]

This is tangential to the current CL, but I wonder if it's really worth having a
lock for sparse histograms, rather than requiring them to always be called from
a single thread...  It's rather atypical for Chrome objects to provide thread
safety, and it doesn't seem obviously necessary for this class to buck the
trend.  Would be good to discuss this more (probably not as part of this
CL/review).

[kaiwang, 2012/10/01 22:38:52]

At least for existing histograms, it's common to be called on different threads.
Also SparseHistogram does not mean to be fast (a major goal of Histogram).
As a "locked histogram" was originally Jim's idea. Jim, what's your thought?

 
-  DISALLOW_COPY_AND_ASSIGN(SampleVector);
+  DISALLOW_COPY_AND_ASSIGN(SampleMap);
 };
 
-class BASE_EXPORT_PRIVATE SampleVectorIterator : public SampleCountIterator {
+class BASE_EXPORT_PRIVATE SampleMapIterator : public SampleCountIterator {
  public:
-  SampleVectorIterator(const std::vector<HistogramBase::Count>* counts,
-                       const BucketRanges* bucket_ranges);
+  SampleMapIterator(
+      const std::map<HistogramBase::Sample, HistogramBase::Count>&
+          sample_count);

[Ilya Sherman, 2012/10/01 21:24:50]

nit: Same naming comment as above.

[kaiwang, 2012/10/01 22:38:52]

Done.

+
+  virtual ~SampleMapIterator();
 
   // SampleCountIterator implementation:
   virtual bool Done() const OVERRIDE;
@@ -65,17 +59,12 @@ class BASE_EXPORT_PRIVATE SampleVectorIterator : public SampleCountIterator {
   virtual void Get(HistogramBase::Sample* min,
                    HistogramBase::Sample* max,
                    HistogramBase::Count* count) const OVERRIDE;
-  virtual bool GetBucketIndex(size_t* index) const OVERRIDE;
-
  private:
-  void SkipEmptyBuckets();
-
-  const std::vector<HistogramBase::Count>* counts_;
-  const BucketRanges* bucket_ranges_;
-
-  size_t index_;
+  std::map<HistogramBase::Sample, HistogramBase::Count>::const_iterator iter_;
+  const std::map<HistogramBase::Sample, HistogramBase::Count>::const_iterator
+      end_;
 };
 
 }  // namespace base
 
-#endif  // BASE_METRICS_SAMPLE_VECTOR_H_
+#endif  // BASE_METRICS_SAMPLE_MAP_H_