Add SampleMap and use it in SparseHistogram

base/metrics/sample_map.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.
 
 // 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,
                           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 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:
+  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.

 
- private:
-  FRIEND_TEST_ALL_PREFIXES(HistogramTest, CorruptSampleCounts);
+  // 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?

 
-  std::vector<HistogramBase::Count> counts_;
-
-  // Shares the same BucketRanges with Histogram object.
-  const BucketRanges* const bucket_ranges_;
-
-  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;
   virtual void Next() OVERRIDE;
   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_