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..d2ded468da2179de235e811fdd50ad6fa54b497d 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,62 @@ 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);
+
+  base::FilePath metrics_dir;
+  if (!base::PathService::Get(chrome::DIR_USER_DATA, &metrics_dir)) {
+    NOTREACHED();
+    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")

[Alexei Svitkine (slow), 2016/06/16 11:36:57]

Please don't use == to compare groups. Best practice is to use StartsWith() or
use variation params instead.

Actually would recommend to use params here, since you already use a feature. So
you can use GetVariationParamsByFeature().

[bcwhite, 2016/06/16 14:21:08]

I started down that path but found it more complicated and still required an
arbitrary string comparison.

BTW, is there a particular reason why the "name" isn't available from the
Feature API?  Is it worth adding?

[Alexei Svitkine (slow), 2016/06/16 14:28:02]

String =='s is fine for variations params, since that still allows you to have
differently named groups set the same params.
Do you mean field trial name? Because you're using name here. :)

[bcwhite, 2016/06/16 14:31:43]

I meant the experiment name as in "Enabled", "Disable", "MemoryMappedFile", etc.

[Alexei Svitkine (slow), 2016/06/16 14:34:40]

Because we're discouraging using experiment name to make decisions, in favor of
params. :)

(Because it's error prone if you use == and then you realise you have data bias
because you rolled your Enabled group to 100% on dogfood and 50/50 for everyone
else and then your Enabled vs. Disabled comparison is completely biased on
platforms when dogfooders are a large chunk of the Enabled users... If you use
params, then you can use different group names and avoid this problem.)

+    return;
+
+  // 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)
+        kAllocSize, kAllocId,
         ChromeMetricsServiceClient::kBrowserMetricsName);
-    base::GlobalHistogramAllocator* allocator =
-        base::GlobalHistogramAllocator::Get();
-    allocator->CreateTrackingHistograms(
+  } else if (feature_group == "MappedFile") {
+    // Create global allocator with the "active" file.
+    base::GlobalHistogramAllocator::CreateWithFile(
+        active_file, kAllocSize, kAllocId,
         ChromeMetricsServiceClient::kBrowserMetricsName);
+  } else {
+    // Shouldn't happen but fail gracefully if it does.
+    NOTREACHED() << feature_group;
+    return;
   }
+
+  base::GlobalHistogramAllocator* allocator =
+      base::GlobalHistogramAllocator::Get();
+  allocator->CreateTrackingHistograms(
+      ChromeMetricsServiceClient::kBrowserMetricsName);
+  allocator->SetPersistentLocation(active_file);
 }
 
 }  // namespace