Break global impact of PersistentHistogramAllocator into a separate class.

base/metrics/persistent_histogram_allocator.h

Index: base/metrics/persistent_histogram_allocator.h
diff --git a/base/metrics/persistent_histogram_allocator.h b/base/metrics/persistent_histogram_allocator.h
index cc8d0233d3d337456651fe9ae9cdadca82d22f0f..9e589faed7583bd6c46f1beb3d24eabbdc2a1358 100644
--- a/base/metrics/persistent_histogram_allocator.h
+++ b/base/metrics/persistent_histogram_allocator.h
@@ -99,50 +99,27 @@ class BASE_EXPORT PersistentHistogramAllocator {
   void CreateTrackingHistograms(StringPiece name);
   void UpdateTrackingHistograms();
 
-  // Manage a PersistentHistogramAllocator for globally storing histograms in
-  // a space that can be persisted or shared between processes. There is only
-  // ever one allocator for all such histograms created by a single process.
-  // This takes ownership of the object and should be called as soon as
-  // possible during startup to capture as many histograms as possible and
-  // while operating single-threaded so there are no race-conditions.
-  static void SetGlobalAllocator(
-      scoped_ptr<PersistentHistogramAllocator> allocator);
-  static PersistentHistogramAllocator* GetGlobalAllocator();
-
-  // This access to the persistent allocator is only for testing; it extracts
-  // the current allocator completely. This allows easy creation of histograms
-  // within persistent memory segments which can then be extracted and used
-  // in other ways.
-  static scoped_ptr<PersistentHistogramAllocator>
-  ReleaseGlobalAllocatorForTesting();
-
-  // These helper methods perform SetGlobalAllocator() calls with allocators
-  // of the specified type and parameters.
-  static void CreateGlobalAllocatorOnPersistentMemory(
-      void* base,
-      size_t size,
-      size_t page_size,
-      uint64_t id,
-      StringPiece name);
-  static void CreateGlobalAllocatorOnLocalMemory(
-      size_t size,
-      uint64_t id,
-      StringPiece name);
-  static void CreateGlobalAllocatorOnSharedMemory(
-      size_t size,
-      const SharedMemoryHandle& handle);
-
-  // Import new histograms from the global PersistentHistogramAllocator. It's
-  // possible for other processes to create histograms in the active memory
-  // segment; this adds those to the internal list of known histograms to
-  // avoid creating duplicates that would have to be merged during reporting.
-  // Every call to this method resumes from the last entry it saw; it costs
-  // nothing if nothing new has been added.
-  static void ImportGlobalHistograms();
-
   // Histogram containing creation results. Visible for testing.
   static HistogramBase* GetCreateHistogramResultHistogram();
 
+ protected:
+  // The structure used to hold histogram data in persistent memory. It is
+  // defined and used entirely within the .cc file.
+  struct PersistentHistogramData;
+
+  // The memory allocator that provides the actual histogram storage.
+  scoped_ptr<PersistentMemoryAllocator> memory_allocator_;

[Alexei Svitkine (slow), 2016/04/01 19:52:40]

Members shouldn't be protected per style guide. ("Note that data members should
be private.").

[bcwhite, 2016/04/04 18:47:29]

Done.

+
+  // A reference to the last-created histogram in the allocator, used to avoid
+  // trying to import what was just created.
+  // TODO(bcwhite): Change this to std::atomic<PMA::Reference> when available.
+  subtle::Atomic32 last_created_ = 0;
+
+  // Get the next histogram in persistent data based on iterator while
+  // ignoring a particular reference if it is found.
+  scoped_ptr<HistogramBase> GetNextHistogramWithIgnore(Iterator* iter,
+                                                       Reference ignore);
+
  private:
   // Enumerate possible creation results for reporting.
   enum CreateHistogramResultType {
@@ -180,16 +157,6 @@ class BASE_EXPORT PersistentHistogramAllocator {
     CREATE_HISTOGRAM_MAX
   };
 
-  // The structure used to hold histogram data in persistent memory. It is
-  // defined and used entirely within the .cc file.
-  struct PersistentHistogramData;
-
-  // Get the next histogram in persistent data based on iterator while
-  // ignoring a particular reference if it is found.
-  scoped_ptr<HistogramBase> GetNextHistogramWithIgnore(
-      Iterator* iter,
-      Reference ignore);
-
   // Create a histogram based on saved (persistent) information about it.
   scoped_ptr<HistogramBase> CreateHistogram(
       PersistentHistogramData* histogram_data_ptr);
@@ -197,14 +164,76 @@ class BASE_EXPORT PersistentHistogramAllocator {
   // Record the result of a histogram creation.
   static void RecordCreateHistogramResult(CreateHistogramResultType result);
 
-  // The memory allocator that provides the actual histogram storage.
-  scoped_ptr<PersistentMemoryAllocator> memory_allocator_;
+  DISALLOW_COPY_AND_ASSIGN(PersistentHistogramAllocator);
+};
 
-  // A reference to the last-created histogram in the allocator, used to avoid
-  // trying to import what was just created.
-  subtle::AtomicWord last_created_ = 0;
 
-  DISALLOW_COPY_AND_ASSIGN(PersistentHistogramAllocator);
+class BASE_EXPORT GlobalHistogramAllocator

[Alexei Svitkine (slow), 2016/04/01 19:52:40]

Can this be a separate file?

Also, add a comment.

[bcwhite, 2016/04/01 20:24:16]

I could, but I'd rather not.  It's very closely tied; separating them would, I
believe, make it more difficult to understand.

[bcwhite, 2016/04/04 18:47:29]

Done.

+    : public PersistentHistogramAllocator {
+ public:
+  ~GlobalHistogramAllocator();

[Alexei Svitkine (slow), 2016/04/01 19:52:40]

Where's the dtor? Did you mean to make it private? If you don't declare one, the
compiler will add an implicit one.

[bcwhite, 2016/04/01 20:24:16]

Objects of this class are passed by scoped_ptr so the dtor has to be public.

+
+  // Create a global allocator using the passed-in memory |base|, |size|, and
+  // other parameters. Ownership of the memory segment remains with the caller.
+  static void CreateWithPersistentMemory(void* base,
+                                         size_t size,
+                                         size_t page_size,
+                                         uint64_t id,
+                                         StringPiece name);
+
+  // Create a global allocator using an internal block of memory of the
+  // specified |size| taken from the heap.
+  static void CreateWithLocalMemory(size_t size, uint64_t id, StringPiece name);
+
+  // Create a global allocator using a block of shared |memory| of the
+  // specified |size|. The allocator takes ownership of the shared memory
+  // and releases it upon destruction, though the memory will continue to
+  // live if other processes have access to it.
+  static void CreateWithSharedMemory(scoped_ptr<SharedMemory> memory,
+                                     size_t size,
+                                     uint64_t id,
+                                     StringPiece name);
+
+  // Create a global allocator using a block of shared memory accessed
+  // through the given |handle| and |size|. The allocator takes ownership
+  // of the handle and closes it upon destruction, though the memory will
+  // continue to live if other processes have access to it.
+  static void CreateWithSharedMemoryHandle(const SharedMemoryHandle& handle,
+                                           size_t size);
+
+  // Manage a GlobalHistogramAllocator for globally storing histograms in
+  // a space that can be persisted or shared between processes. There is only
+  // ever one allocator for all such histograms created by a single process.
+  // This takes ownership of the object and should be called as soon as
+  // possible during startup to capture as many histograms as possible and
+  // while operating single-threaded so there are no race-conditions.
+  static void Set(scoped_ptr<GlobalHistogramAllocator> allocator);
+  static GlobalHistogramAllocator* Get();

[Alexei Svitkine (slow), 2016/04/01 19:52:40]

Split these functions with a comment for Get() - otherwise it's hard to read -
i.e. from a first glance it may look like a wrapped line.

[bcwhite, 2016/04/04 18:47:29]

Done.

+
+  // This access to the persistent allocator is only for testing; it extracts
+  // the current allocator completely. This allows easy creation of histograms
+  // within persistent memory segments which can then be extracted and used
+  // in other ways.
+  static scoped_ptr<GlobalHistogramAllocator> ReleaseForTesting();
+
+ private:
+  friend class StatisticsRecorder;
+
+  GlobalHistogramAllocator(scoped_ptr<PersistentMemoryAllocator> memory);
+
+  // Import new histograms from the global histogram allocator. It's possible
+  // for other processes to create histograms in the active memory segment;
+  // this adds those to the internal list of known histograms to avoid creating
+  // duplicates that would have to be merged during reporting. Every call to
+  // this method resumes from the last entry it saw; it costs nothing if
+  // nothing new has been added.
+  void ImportHistogramsToStatisticsRecorder();
+
+  // Import always continues from where it left off, making use of a single
+  // iterator to continue the work.
+  Iterator import_iterator_;
+
+  DISALLOW_COPY_AND_ASSIGN(GlobalHistogramAllocator);
 };
 
 }  // namespace base