Improve efficiency of persistent sparse histograms.

base/metrics/persistent_sample_map.h

Index: base/metrics/persistent_sample_map.h
diff --git a/base/metrics/persistent_sample_map.h b/base/metrics/persistent_sample_map.h
index 4ae54d8ae26f79af86d010deedc057fa759e3840..bea94c3b0681fbc318287e6578bd50f42c94beaf 100644
--- a/base/metrics/persistent_sample_map.h
+++ b/base/metrics/persistent_sample_map.h
@@ -22,13 +22,28 @@
 
 namespace base {
 
+class PersistentHistogramAllocator;
+class PersistentSampleMapRecords;
+class PersistentSparseHistogramHelper;
+
 // The logic here is similar to that of SampleMap but with different data
 // structures. Changes here likely need to be duplicated there.
 class BASE_EXPORT PersistentSampleMap : public HistogramSamples {
  public:
+  // Constructs a persistent sample map using any of a variety of persistent
+  // data sources. Really, the first two are just convenience methods for
+  // getting at the PersistentSampleMapRecords object for the specified |id|.
+  // The source objects must live longer than this object.
+  PersistentSampleMap(uint64_t id,
+                      PersistentHistogramAllocator* allocator,
+                      Metadata* meta);
+  PersistentSampleMap(uint64_t id,
+                      PersistentSparseHistogramHelper* helper,
+                      Metadata* meta);
   PersistentSampleMap(uint64_t id,
-                      PersistentMemoryAllocator* allocator,
+                      PersistentSampleMapRecords* records,
                       Metadata* meta);

[Alexei Svitkine (slow), 2016/04/13 15:53:50]

Do we really need all 3? I think it would be useful to discuss why all the
different types are needed. I would possibly vote for simplifying this to avoid
needing all the different permutations.

Specifically, it seems the last one does a Reset() which should definitely be
documented if we're keeping that version.

[bcwhite, 2016/04/13 22:45:04]

The one taking a PersistentSampleMapRecords is the real constructor that does
the work.

The one taking a PersitentSparseHistogramHelper is used by tests which need to
validate sample sharing between multiple instances of the same histogram (like
having multiple processes).  This could be removed and the test altered to use
duplicate allocators instead of independent helpers.

The one taking a PersistentHistogramAllocator simplifies the interface and means
that the SparseHistogram class doesn't have to include the full
persistent_histogram_allocator.h file just to call GetSampleMapRecords() off of
the passed-in allocator object.
They all do exactly the same thing.  The first two just dereference the middle
parameter before "delegating" to the third one.

+
   ~PersistentSampleMap() override;
 
   // HistogramSamples:
@@ -38,6 +53,20 @@ class BASE_EXPORT PersistentSampleMap : public HistogramSamples {
   HistogramBase::Count TotalCount() const override;
   std::unique_ptr<SampleCountIterator> Iterator() const override;
 
+  // Uses a persistent-memory |iterator| to locate and return information about
+  // the next record holding information for a PersistentSampleMap. The record
+  // could be for any Map so return the |sample_map_id| as well.
+  static PersistentMemoryAllocator::Reference GetNextPersistentRecord(
+      PersistentMemoryAllocator::Iterator& iterator,
+      uint64_t* sample_map_id);
+
+  // Creates a new record in an |allocator| storing count information for a
+  // specific sample |value| of a histogram with the given |sample_map_id|.
+  static PersistentMemoryAllocator::Reference CreatePersistentRecord(
+      PersistentMemoryAllocator* allocator,
+      uint64_t sample_map_id,
+      HistogramBase::Sample value);
+
  protected:
   // Performs arithemetic. |op| is ADD or SUBTRACT.
   bool AddSubtractImpl(SampleCountIterator* iter, Operator op) override;
@@ -63,12 +92,14 @@ class BASE_EXPORT PersistentSampleMap : public HistogramSamples {
   HistogramBase::Count* ImportSamples(HistogramBase::Sample until_value);
 
   // All created/loaded sample values and their associated counts. The storage
-  // for the actual Count numbers is owned by the |allocator_|.
+  // for the actual Count numbers is owned by the |records_| object and its
+  // underlying allocator.
   std::map<HistogramBase::Sample, HistogramBase::Count*> sample_counts_;
 
-  // The persistent memory allocator holding samples and an iterator through it.
-  PersistentMemoryAllocator* allocator_;
-  PersistentMemoryAllocator::Iterator sample_iter_;
+  // The object that manages records inside persistent memory. This is owned
+  // externally (typically by a PersistentHistogramAllocator) and is expected
+  // to live beyond the life of this object.
+  PersistentSampleMapRecords* records_;
 
   DISALLOW_COPY_AND_ASSIGN(PersistentSampleMap);
 };