Support experiment to store persistent metrics in memory-mapped file.

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 e63dfea0c464edd0fc8efa3d960487181d4d9a42..adf15d93c78d5a028b34a56fda0bd99b47c74f76 100644
--- a/chrome/browser/chrome_browser_field_trials.cc
+++ b/chrome/browser/chrome_browser_field_trials.cc
@@ -8,14 +8,17 @@
 
 #include "base/command_line.h"
 #include "base/feature_list.h"
+#include "base/files/file_util.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/browser/metrics/chrome_metrics_service_client.h"
 #include "chrome/browser/tracing/background_tracing_field_trial.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "components/metrics/metrics_pref_names.h"
 
@@ -30,20 +33,61 @@ namespace {
 // Check for feature enabling the use of persistent histogram storage and
 // enable the global allocator if so.
 void InstantiatePersistentHistograms() {
-  if (base::FeatureList::IsEnabled(base::kPersistentHistogramsFeature)) {
-    // 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, as reported in UMA, peaked around 1.9MiB
-    // as of 2016-02-20.
+  const std::string feature_group = base::FieldTrialList::FindFullName(
+      base::kPersistentHistogramsFeature.name);

[Ilya Sherman, 2016/06/17 19:54:32]

nit: Please move this down to where it's used.

+
+  base::FilePath metrics_dir;
+  if (!base::PathService::Get(chrome::DIR_USER_DATA, &metrics_dir)) {
+    NOTREACHED();

[Ilya Sherman, 2016/06/17 19:54:32]

nit: Chrome style guidelines recommend either a NOTREACHED() or an early return,
but not both.  In this case, an early return is probably more appropriate, since
the filesystem can get corrupted or fail in other ways.

[bcwhite, 2016/06/20 16:21:28]

Done.

+    return;
+  }
+
+  base::FilePath metrics_file =
+      metrics_dir
+          .AppendASCII(ChromeMetricsServiceClient::kBrowserMetricsName)
+          .AddExtension(base::PersistentMemoryAllocator::kFileExtension);
+  base::FilePath active_file =
+      metrics_dir
+          .AppendASCII(
+              std::string(ChromeMetricsServiceClient::kBrowserMetricsName) +
+              "-active")
+          .AddExtension(base::PersistentMemoryAllocator::kFileExtension);
+
+  // Move any existing "active" file to the final name from which it will be
+  // read when reporting initial stability metrics. If there is no file to
+  // move, remove any old, existing file from before the previous session.
+  if (!base::ReplaceFile(active_file, metrics_file, nullptr))
+    base::DeleteFile(metrics_file, /*recursive=*/false);
+
+  // Stop here if the feature is not enabled.
+  if (feature_group.empty() || feature_group == "Disabled")
+    return;

[Ilya Sherman, 2016/06/17 19:54:32]

Please use the FeatureList API to test whether the feature is enabled, so that
enabling/disabling via the command-line works.

+
+  // 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, as reported in UMA, peaked around 1.9MiB
+  // as of 2016-02-20.
+  const size_t kAllocSize = 3 << 20;     // 3 MiB
+  const uint32_t kAllocId = 0x935DDD43;  // SHA1(BrowserMetrics)
+  if (feature_group == "Enabled") {
     base::GlobalHistogramAllocator::CreateWithLocalMemory(
-        3 << 20,     // 3 MiB
-        0x935DDD43,  // SHA1(BrowserMetrics)
-        ChromeMetricsServiceClient::kBrowserMetricsName);
-    base::GlobalHistogramAllocator* allocator =
-        base::GlobalHistogramAllocator::Get();
-    allocator->CreateTrackingHistograms(
+        kAllocSize, kAllocId, ChromeMetricsServiceClient::kBrowserMetricsName);
+  } else if (feature_group == "MappedFile") {
+    // Create global allocator with the "active" file.
+    base::GlobalHistogramAllocator::CreateWithFile(
+        active_file, kAllocSize, kAllocId,
         ChromeMetricsServiceClient::kBrowserMetricsName);

[Ilya Sherman, 2016/06/17 19:54:32]

I *think* the latest advice that I saw about using multiple experimental states
in conjunction with the base::FeatureList API is to have a single Enabled group,
and then to use field trial params to control the parameters of how the trial is
enabled.  Alexei knows this better than I do, though.

[Ilya Sherman, 2016/06/17 19:57:23]

Ah, I actually wrote this before going back and reading the comment history on
the CL.  Seems like Alexei said roughly the same thing =)

[bcwhite, 2016/06/20 16:21:28]

Done.

+  } else {
+    // Shouldn't happen but fail gracefully if it does.
+    NOTREACHED() << feature_group;
+    return;

[Ilya Sherman, 2016/06/17 19:54:32]

nit: Ditto about NOTREACHED() or early return.  In this case, I'd actually omit
both -- there's no reason for this code path to be reachable.  Or, if you're
worried about corruption (?), then an early return seems like the right choice.

   }
+
+  base::GlobalHistogramAllocator* allocator =
+      base::GlobalHistogramAllocator::Get();
+  allocator->CreateTrackingHistograms(
+      ChromeMetricsServiceClient::kBrowserMetricsName);
+  allocator->SetPersistentLocation(active_file);
 }
 
 }  // namespace