Support "shared" histograms between processes.

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 b38abad373b6044c22287436cbbee1a4db543a4c..bacc47cb9fefd7d8932a1624f593d23cf43193c8 100644
--- a/chrome/browser/chrome_browser_field_trials.cc
+++ b/chrome/browser/chrome_browser_field_trials.cc
@@ -7,7 +7,10 @@
 #include <string>
 
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/metrics/field_trial.h"
+#include "base/metrics/histogram_base.h"
+#include "base/metrics/histogram_persistence.h"
 #include "base/strings/string_util.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
@@ -41,6 +44,18 @@ void ChromeBrowserFieldTrials::SetupFieldTrials() {
 }
 
 void ChromeBrowserFieldTrials::InstantiateDynamicTrials() {
+  // Persistent histograms must be enabled as soon as possible.
+  if (base::FeatureList::IsEnabled(base::kPersistentHistogramsFeature)) {

[Alexei Svitkine (slow), 2016/01/14 16:43:14]

You define the feature in base/ but only query it in chrome/browser. I don't
think this makes sense.

If we want the feature to only control the browser process' use of persistence,
let's define it in chrome/browser - e.g. in this file. We can then have a
separate feature to control renderers. We should also put Browser in the name
somewhere to make it clear what it controls.

[bcwhite, 2016/01/14 19:20:26]

I want the same parameter to control all.  I just haven't gotten to writing the
Renderer part of it.

[Alexei Svitkine (slow), 2016/01/14 21:53:41]

I thought we weren't certain if we wanted the browser process to use the new
system for its own histograms and we only wanted this feature control to test
out that things are working as expected?

If that's still true, it sounds like we should have separate features.

OTOH, if we do want them to share it, then I suggest making the Feature local to
the base code and have a function in base that sets it up (w/ appropriate
params). I know you mentioned that the renderer use case will use a different
way of initialization the allocator - but that's OK, we can just have a separate
function for that as well. That is, assuming we want the same feature to be used
for both, which I'm not convinced of (given what I mention above).

[bcwhite, 2016/01/15 15:24:38]

There's no persistence on the Browser side but moving the histograms into a
persistent memory segment (even one not persisted) will give immediate feedback
on operation and performance without depending on all the sharing code.
I don't see that as the case.  Like any large feature, testers just get more of
it as development continues.
My point is that common code simply doesn't know enough.  Which persistent
allocator is created?  Where does the underlying memory segment come from?  Or
was it an underlying file?  You'd end up with a half-dozen methods all with
about 3 lines of code with each being distinct from the other.  And then the
caller would still have do almost all the same work in order to get the pieces
together.

I'll keep it in mind, though.  If it becomes evident at a later time how to make
much of it common, I'll move it then.

+    const std::string allocator_name("BrowserMetricsAllocator");
+    // Create persistent/shared memory and allow histograms to be stored in it.
+    // TODO(bcwhite): Update this with correct allocator and memory size.
+    base::SetPersistentHistogramMemoryAllocator(
+        new base::LocalPersistentMemoryAllocator(1 << 20,     // 1 MiB
+                                                 0x4D5B9953,  // SHA1(B..M..A..)
+                                                 allocator_name));
+    base::GetPersistentHistogramMemoryAllocator()->CreateHistograms(
+        allocator_name);
+  }

[Alexei Svitkine (slow), 2016/01/14 16:43:14]

Can you move this new code to a helper function?

[bcwhite, 2016/01/14 19:20:26]

I looked at that and it would have to be a helper function defined in this same
file because this is the only place that knows what type of allocator to create.
 A renderer would create it entirely differently and there's no way to pass that
methodology via parameters.

[Alexei Svitkine (slow), 2016/01/18 19:25:35]

Okay, please at least make it an anon function at the top of the file, so that
InstantiateDynamicTrials() doesn't have this logic and just calls it. (Otherwise
if each trial will clutter this function with its logic, it gets out of hand.)

[bcwhite, 2016/01/20 15:26:14]

Done.

   // The following trials are used from renderer process.
   // Mark here so they will be sync-ed.
   base::FieldTrialList::FindValue("DisplayList2dCanvas");