Support saving browser metrics to disk and reading them during next run.

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 7f46dc9664bd6a45d3c010f8cebd100e9ddce25a..3db91985d0c5cc419d0f1204593f65daf3aaae4b 100644
--- a/base/metrics/persistent_histogram_allocator.h
+++ b/base/metrics/persistent_histogram_allocator.h
@@ -389,18 +389,46 @@ class BASE_EXPORT GlobalHistogramAllocator
   // while operating single-threaded so there are no race-conditions.
   static void Set(std::unique_ptr<GlobalHistogramAllocator> allocator);
 
-  // Gets a pointer to the global histogram allocator.
+  // Enables the global persistent allocator. Existing histograms will not be
+  // affected; only newly created ones will go into the allocator.
+  static void Enable();
+
+  // Disables the global persistent allocator. Existing histograms will not be
+  // affected; newly created histograms will be allocated from the heap.
+  static void Disable();

[Ilya Sherman, 2016/04/25 19:48:40]

Rather than having both an Enable() and a Disable() method, I'd prefer to just
have a single Enable() method, since disabled is the default state.

[bcwhite, 2016/04/25 20:37:17]

The default is "enabled" because that is the normal way of operating.  Only this
special case of an experiment creates the allocator but doesn't use it.

I could change it to "Enabled(bool enable)" if you prefer.

[Ilya Sherman, 2016/04/25 21:12:11]

Sorry, I don't follow.  How is the experiment a special case?  There's only a
single allocator, and it's currently disabled by default, and enabled for X% of
users (for some value of X).  Am I understanding that correctly?

[bcwhite, 2016/04/26 13:32:30]

See discussion in persistent_histogram_allocator.cc...

[Ilya Sherman, 2016/04/26 20:01:24]

Okay, yeah, I see your point.  Thanks.

+
+  // Returns whether a global allocator is enabled. This will return false if
+  // none has been set.
+  static bool IsEnabled();

[Ilya Sherman, 2016/04/25 19:48:40]

This method appears to be unused.

[bcwhite, 2016/04/25 20:37:17]

I was using it...  Code must have changed.  Still, seems like it's good to be
symmetrical.

[Ilya Sherman, 2016/04/25 21:12:11]

I strongly disagree that it's worthwhile to land dead code.  This adds work for
the compiler, for maintainers of the code, etc., with no clear benefits.  If the
code is ever needed, we can add it at that time.

[bcwhite, 2016/04/26 13:32:30]

Done.

+
+  // Gets a pointer to an enabled global histogram allocator. If a global
+  // allocator exists but is disabled, this will return null.
   static GlobalHistogramAllocator* Get();
 
+  // Gets a pointer to a global histogram allocator if one exists, even if
+  // that allocator is disabled.
+  static GlobalHistogramAllocator* GetEvenIfDisabled();

[Ilya Sherman, 2016/04/25 19:48:40]

IMO this method is not needed.  Get() should always return the allocator, and
the enabled/disabled state should control how the allocator behaves.  That said,
it's fairly surprising that some code expects to work with the allocator even
when it's disabled.  Is enabled/disabled the wrong metaphor here?  Or should the
code that tries to use the allocator when its disabled be refactored so that it
uses some other object, which is always enabled?

[bcwhite, 2016/04/25 20:37:17]

Get() always returns an active allocator.  Existing code makes this call to know
whether to allocate persistantly or not.  If there is no persistent allocator,
this returns null.  Disabling the allocator acheives the same result.

However, there needs to be a way to get an allocator (even if disabled) if one
exists in order to call other methods on it.  It's not uncommen for objects to
be accessed for some things even when they're not "active" in their primary
role.

[Ilya Sherman, 2016/04/25 21:12:11]

IMO this is a poor design pattern, and I'd much rather separate out those roles
into distinct classes.  I'll admit that I haven't deeply thought about the
specific method calls involved here; so if you disagree, please feel free to
explain to me why this design makes the most sense here.

[bcwhite, 2016/04/26 13:32:30]

The whole GlobalHistogramAllocator is already separate out from the main class
to support all things "global".  

Once the experiment is over, this call can be removed along with enable/disable.

+
   // 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.
+  // the current allocator completely regardless whether it is enabled or not.
+  // This allows easy creation of histograms within persistent memory segments
+  // which can then be extracted and used in other ways.
   static std::unique_ptr<GlobalHistogramAllocator> ReleaseForTesting();
 
+  // Stores a pathname to which the contents of this allocator should be saved
+  // in order to persist the data for a later use.
+  void SetPersistentLocation(const FilePath& location);
+
+  // Writes the internal data to a previously set location. This is generally
+  // called when a process is exiting from a section of code that may not know
+  // the filesystem. The data is written in an atomic manner. The return value
+  // indicates success.
+  bool WriteToPersistentLocation();
+
  private:
   friend class StatisticsRecorder;
 
+  // Creates a new global histogram allocator. It will be enabled by default.
   explicit GlobalHistogramAllocator(
       std::unique_ptr<PersistentMemoryAllocator> memory);
 
@@ -416,6 +444,9 @@ class BASE_EXPORT GlobalHistogramAllocator
   // iterator to continue the work.
   Iterator import_iterator_;
 
+  // The location to which the data should be persisted.
+  FilePath persistent_location_;
+
   DISALLOW_COPY_AND_ASSIGN(GlobalHistogramAllocator);
 };