Break global impact of PersistentHistogramAllocator into a separate class.

base/metrics/persistent_histogram_allocator.h

 // Copyright 2016 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.
 
 #ifndef BASE_METRICS_HISTOGRAM_PERSISTENCE_H_
 #define BASE_METRICS_HISTOGRAM_PERSISTENCE_H_
 
 #include "base/atomicops.h"
 #include "base/base_export.h"
 #include "base/feature_list.h"
@@ skipping 21 matching lines @@
 
     // The iterator used for stepping through persistent memory iterables.
     PersistentMemoryAllocator::Iterator memory_iter;
   };
 
   using Reference = PersistentMemoryAllocator::Reference;
 
   // A PersistentHistogramAllocator is constructed from a PersistentMemory-
   // Allocator object of which it takes ownership.
   PersistentHistogramAllocator(scoped_ptr<PersistentMemoryAllocator> memory);
   ~PersistentHistogramAllocator();

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

This should be virtual since there is a subclass.

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

Done.

 
   // Direct access to underlying memory allocator. If the segment is shared
   // across threads or processes, reading data through these values does
   // not guarantee consistency. Use with care. Do not write.
   PersistentMemoryAllocator* memory_allocator() {
     return memory_allocator_.get();
   }
 
   // Implement the "metadata" API of a PersistentMemoryAllocator, forwarding
   // those requests to the real one.
@@ skipping 39 matching lines @@
   // is done seperately from construction for situations such as when the
   // histograms will be backed by memory provided by this very allocator.
   //
   // IMPORTANT: Callers must update tools/metrics/histograms/histograms.xml
   // with the following histograms:
   //    UMA.PersistentAllocator.name.Allocs
   //    UMA.PersistentAllocator.name.UsedPct
   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 {
     // Everything was fine.
     CREATE_HISTOGRAM_SUCCESS = 0,
 
     // Pointer to metadata was not valid.
     CREATE_HISTOGRAM_INVALID_METADATA_POINTER,
 
     // Histogram metadata was not valid.
@@ skipping 17 matching lines @@
     // Histogram was of unknown type.
     CREATE_HISTOGRAM_UNKNOWN_TYPE,
 
     // Instance has detected a corrupt allocator (recorded only once).
     CREATE_HISTOGRAM_ALLOCATOR_NEWLY_CORRUPT,
 
     // Always keep this at the end.
     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);
 
   // 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_;
-
-  // 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
 
 #endif  // BASE_METRICS_HISTOGRAM_PERSISTENCE_H_