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

chrome/browser/chrome_browser_field_trials.cc

Index: chrome/browser/chrome_browser_field_trials.cc
diff --git a/chrome/browser/chrome_browser_field_trials.cc b/chrome/browser/chrome_browser_field_trials.cc
index 648167695f6b70b9ca8b5c986e7c773d7d08c2ca..38ac04cc8d970049060509cdb3a2c793d398e0df 100644
--- a/chrome/browser/chrome_browser_field_trials.cc
+++ b/chrome/browser/chrome_browser_field_trials.cc
@@ -8,12 +8,15 @@
 
 #include "base/command_line.h"
 #include "base/feature_list.h"
+#include "base/files/file_path.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_base.h"
 #include "base/metrics/persistent_histogram_allocator.h"
+#include "base/path_service.h"
 #include "base/strings/string_util.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "components/metrics/metrics_pref_names.h"
 
@@ -26,19 +29,14 @@
 namespace {
 
 // Check for feature enabling the use of persistent histogram storage and
-// create an appropriate allocator for such if so.
+// enable the global allocator if so.
 void InstantiatePersistentHistograms() {
   if (base::FeatureList::IsEnabled(base::kPersistentHistogramsFeature)) {
-    const char kAllocatorName[] = "BrowserMetrics";
-    // Create persistent/shared memory and allow histograms to be stored in it.
-    // Memory that is not actualy used won't be physically mapped by the system.
-    // BrowserMetrics usage peaked around 95% of 2MiB as of 2016-02-20.
-    base::GlobalHistogramAllocator::CreateWithLocalMemory(
-        3 << 20,     // 3 MiB
-        0x935DDD43,  // SHA1(BrowserMetrics)
-        kAllocatorName);
-    base::GlobalHistogramAllocator::Get()->CreateTrackingHistograms(
-        kAllocatorName);
+    base::GlobalHistogramAllocator::Enable();

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

It seems odd to have this as a static method.  Why not just Get() the allocator
below, and then call Enable() on it?

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

I considered both.

The general differentiation has been that methods that operate on the allocator
object are normal while methods that join it to the global (singleton) scope are
static.

The "global" allocator object knows nothing about whether it's tied into the
metrics system, just that it needs some additional code to support that role.

Enable/Disable doesn't need any knowledge from inside a GlobalHistogramAllocator
object.  It does some management on a global instance of such an object but
makes no call into it.

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

I don't really follow, sorry.  In any case, I don't care so much about whether
the method itself is static or not.  But I do think it makes more sense for the
allocator to track, internally, whether it's enabled or not, rather than
tracking that state via global variables.  Basically, global variables are
harder to reason about than instance variables, so I'd prefer to use instance
variables when possible.

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

My style matches yours but I've been previously asked specifically to use
anonymous globals where possible so that's why it's done this way.

To internally track enabled/disabled would require it added to the
PersistentHistogramAllocator main (base) class even though it doesn't really
make sense to be there.  It would also require additional plumbing in the
callers which generally don't expect such a call to fail; they instead expect
that an allocator may not exist at all (i.e. Get() may return null).

Again, significant complication for something that will likely be removed in the
future.

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

Okay, I agree that if the base PHA class doesn't normally need to track
enabled/disabled state, then an anonymous global is an ok compromise.

+    base::GlobalHistogramAllocator* allocator =
+        base::GlobalHistogramAllocator::Get();
+    DCHECK(allocator);  // Should have been created during startup.
+    allocator->CreateTrackingHistograms(allocator->Name());
   }
 }