Use SharedPersistentMemoryAllocator to share field trial state

base/metrics/field_trial.cc

Index: base/metrics/field_trial.cc
diff --git a/base/metrics/field_trial.cc b/base/metrics/field_trial.cc
index 4f3e0c76fcd58c606f3b98f90718f8dc63538d5f..912f67b7816d02f58072def789004b4f0a1c44be 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -35,9 +35,35 @@ const char kActivationMarker = '*';
 // for now while the implementation is fleshed out (e.g. data format, single
 // shared memory segment). See https://codereview.chromium.org/2365273004/ and
 // crbug.com/653874
-#if defined(OS_WIN)
 const bool kUseSharedMemoryForFieldTrials = false;
-#endif
+
+// Constants for the field trial allocator.
+const char kAllocatorName[] = "FieldTrialAllocator";
+const uint32_t kFieldTrialType = 0xABA17E13 + 1;  // SHA1(FieldTrialEntry) v1
+
+// We create one FieldTrialEntry struct per field trial in shared memory (via
+// field_trial_allocator). It contains whether or not it's activated, and the
+// offset from the start of the allocated memory to the group name. We don't
+// need the offset into the trial name because that's always a fixed position
+// from the start of the struct. Two strings will be appended to the end of this
+// structure in allocated memory, so the result will look like this:
+// ---------------------------------
+// | fte | trial_name | group_name |
+// ---------------------------------
+struct FieldTrialEntry {
+  bool activated;
+  uint32_t group_name_offset;
+
+  char* GetTrialName() const {

[Alexei Svitkine (slow), 2016/10/19 16:32:49]

const char*

Same below.

[lawrencewu, 2016/10/19 20:18:26]

Done.

+    char* src = reinterpret_cast<char*>(const_cast<FieldTrialEntry*>(this));
+    return src + sizeof(FieldTrialEntry);
+  }
+
+  char* GetGroupName() const {
+    char* src = reinterpret_cast<char*>(const_cast<FieldTrialEntry*>(this));
+    return src + this->group_name_offset;
+  }
+};
 
 // Created a time value based on |year|, |month| and |day_of_month| parameters.
 Time CreateTimeFromParams(int year, int month, int day_of_month) {
@@ -132,6 +158,8 @@ const int FieldTrial::kDefaultGroupNumber = 0;
 bool FieldTrial::enable_benchmarking_ = false;
 
 int FieldTrialList::kNoExpirationYear = 0;
+base::SharedPersistentMemoryAllocator* FieldTrialList::field_trial_allocator =
+    nullptr;
 
 //------------------------------------------------------------------------------
 // FieldTrial methods and members.
@@ -577,23 +605,19 @@ void FieldTrialList::CreateTrialsFromCommandLine(
 #if defined(OS_WIN)
   if (cmd_line.HasSwitch(field_trial_handle_switch)) {
     std::string arg = cmd_line.GetSwitchValueASCII(field_trial_handle_switch);
-    size_t token = arg.find(",");
-    int field_trial_handle = std::stoi(arg.substr(0, token));
-    int field_trial_length = std::stoi(arg.substr(token + 1, arg.length()));
+    int field_trial_handle = std::stoi(arg);
 
     HANDLE handle = reinterpret_cast<HANDLE>(field_trial_handle);
     base::SharedMemoryHandle shm_handle =
         base::SharedMemoryHandle(handle, base::GetCurrentProcId());
 
     // Gets deleted when it gets out of scope, but that's OK because we need it
-    // only for the duration of this call currently anyway.
-    base::SharedMemory shared_memory(shm_handle, false);
-    shared_memory.Map(field_trial_length);
+    // only for the duration of this method.
+    std::unique_ptr<base::SharedMemory> shm(
+        new base::SharedMemory(shm_handle, false));
+    shm.get()->Map(4 << 10);  // 4 KiB = one page

[Alexei Svitkine (slow), 2016/10/19 16:32:48]

Shouldn't this be using field_trial_length?

[lawrencewu, 2016/10/19 20:18:26]

I don't think so -- allocated memory isn't guaranteed to be contiguous, so to be
safe we should map the same amount of memory we mapped in the browser process.

[Alexei Svitkine (slow), 2016/10/19 20:39:36]

But that part can be passed from the browser using field_trial_length part of
the command line flag? I'm thinking of a case where we may need more than 4KB of
space - i.e. the browser makes the decision of how big the segment is and tells
child processes this size, rather than child processes assuming they know the
size.

I guess at this point it's moot since it's hardcoded in both places, so if you
want you can add a TODO about it instead.

[lawrencewu, 2016/10/20 17:36:05]

Ah I see, yes that can be passed. Done.

 
-    char* field_trial_state = static_cast<char*>(shared_memory.memory());
-    bool result = FieldTrialList::CreateTrialsFromString(
-        std::string(field_trial_state), std::set<std::string>());
-    DCHECK(result);
+    FieldTrialList::CreateTrialsFromSharedMemory(std::move(shm));
     return;
   }
 #endif
@@ -606,37 +630,95 @@ void FieldTrialList::CreateTrialsFromCommandLine(
   }
 }
 
+#if defined(OS_WIN)
 // static
-std::unique_ptr<base::SharedMemory> FieldTrialList::CopyFieldTrialStateToFlags(
+void FieldTrialList::AppendFieldTrialHandleIfNeeded(
+    base::HandlesToInheritVector& handles) {

[Alexei Svitkine (slow), 2016/10/19 16:32:48]

Parameters shouldn't be passed by non-const refs, per the style guide. Instead,
pass by pointer.

[lawrencewu, 2016/10/19 20:18:26]

Done.

+  // If the allocator has not been created at this point, then instantiate it
+  // and add all the existing field trials.
+  {
+    AutoLock auto_lock(global_->lock_);
+    // Create the allocator if not already created.
+    if (field_trial_allocator == nullptr) {
+      std::unique_ptr<base::SharedMemory> shm(new base::SharedMemory());
+      shm->CreateAndMapAnonymous(4 << 10);  // 4 KiB = one page
+
+      field_trial_allocator = new base::SharedPersistentMemoryAllocator(
+          std::move(shm), 0, kAllocatorName, false);
+      field_trial_allocator->CreateTrackingHistograms(kAllocatorName);
+      // TODO(lawrencewu): call UpdateTrackingHistograms() when all field trials
+      // have been registered (perhaps in the destructor?)
+    }
+  }
+
+  for (const auto& registered : global_->registered_) {
+    FieldTrialList::UpdateFieldTrialAllocator(registered.second);

[Alexei Svitkine (slow), 2016/10/19 16:32:48]

Wouldn't this cause entries to be added multiple times each time the function is
called? If not, please add a comment explaining it.

[lawrencewu, 2016/10/19 20:18:26]

Yes, you're right. I moved this up into where the allocator gets instantiated so
it should only get called once.

[Alexei Svitkine (slow), 2016/10/19 20:39:36]

Can this be covered by a unit test?

[lawrencewu, 2016/10/20 17:36:05]

Done.

+  }
+
+  // Need to get a new, read-only handle to share to the child.
+  HANDLE src = field_trial_allocator->shared_memory()->handle().GetHandle();
+  ProcessHandle process = GetCurrentProcess();
+  DWORD access = SECTION_MAP_READ | SECTION_QUERY;
+  HANDLE dst;
+  ::DuplicateHandle(process, src, process, &dst, access, true, 0);
+  handles.push_back(dst);
+}
+#endif
+
+// static
+void FieldTrialList::CreateTrialsFromSharedMemory(
+    std::unique_ptr<base::SharedMemory> shm) {
+  const base::SharedPersistentMemoryAllocator shalloc(std::move(shm), 0,
+                                                      kAllocatorName, true);
+  base::PersistentMemoryAllocator::Iterator iter(&shalloc);
+
+  base::SharedPersistentMemoryAllocator::Reference ref;
+  while ((ref = iter.GetNextOfType(kFieldTrialType)) !=
+         base::SharedPersistentMemoryAllocator::kReferenceNull) {
+    const FieldTrialEntry* fte =

[Alexei Svitkine (slow), 2016/10/19 16:32:49]

Nit: fte -> entry

as acronyms are discouraged generally.

[lawrencewu, 2016/10/19 20:18:26]

Done.

+        shalloc.GetAsObject<const FieldTrialEntry>(ref, kFieldTrialType);
+    char* trial_name = fte->GetTrialName();
+    char* group_name = fte->GetGroupName();

[Alexei Svitkine (slow), 2016/10/19 16:32:48]

Just inline these into the CreateFieldTrial() call below.

[lawrencewu, 2016/10/19 20:18:25]

Done.

+    FieldTrial* trial = CreateFieldTrial(trial_name, group_name);
+
+    if (fte->activated) {
+      // Call |group()| to mark the trial as "used" and notify observers, if
+      // any. This is useful to ensure that field trials created in child
+      // processes are properly reported in crash reports.
+      trial->group();
+    }
+  }
+}
+
+// static
+void FieldTrialList::CopyFieldTrialStateToFlags(
     const char* field_trial_handle_switch,
     base::CommandLine* cmd_line) {
+#if defined(OS_WIN)
+  // Use shared memory to pass the state if the feature is enabled, otherwise
+  // fallback to passing it via the command line as a string.
+  if (kUseSharedMemoryForFieldTrials) {
+    base::SharedMemory* shm = field_trial_allocator->shared_memory();
+
+    // HANDLE is just typedef'd to void *. We basically cast the handle into an
+    // int (uintptr_t, to be exact), stringify the int, and pass it as a
+    // command-line flag. The child process will do the reverse conversions to
+    // retrieve the handle. See http://stackoverflow.com/a/153077
+    auto uintptr_handle =
+        reinterpret_cast<uintptr_t>(shm->handle().GetHandle());
+    std::string field_trial_handle = std::to_string(uintptr_handle);
+
+    cmd_line->AppendSwitchASCII(field_trial_handle_switch, field_trial_handle);
+    return;
+  }
+#endif
+
   std::string field_trial_states;
   base::FieldTrialList::AllStatesToString(&field_trial_states);
   if (!field_trial_states.empty()) {
-// Use shared memory to pass the state if the feature is enabled, otherwise
-// fallback to passing it via the command line as a string.
-#if defined(OS_WIN)
-    if (kUseSharedMemoryForFieldTrials) {
-      std::unique_ptr<base::SharedMemory> shm(new base::SharedMemory());
-      size_t length = field_trial_states.size() + 1;
-      shm->CreateAndMapAnonymous(length);
-      memcpy(shm->memory(), field_trial_states.c_str(), length);
-
-      // HANDLE is just typedef'd to void *
-      auto uintptr_handle =
-          reinterpret_cast<std::uintptr_t>(shm->handle().GetHandle());
-      std::string field_trial_handle =
-          std::to_string(uintptr_handle) + "," + std::to_string(length);
-
-      cmd_line->AppendSwitchASCII(field_trial_handle_switch,
-                                  field_trial_handle);
-      return shm;
-    }
-#endif
     cmd_line->AppendSwitchASCII(switches::kForceFieldTrials,
                                 field_trial_states);
   }
-  return std::unique_ptr<base::SharedMemory>(nullptr);
 }
 
 // static
@@ -694,12 +776,53 @@ void FieldTrialList::NotifyFieldTrialGroupSelection(FieldTrial* field_trial) {
   if (!field_trial->enable_field_trial_)
     return;
 
+  if (kUseSharedMemoryForFieldTrials) {
+    FieldTrialList::UpdateFieldTrialAllocator(field_trial);
+  }
+
   global_->observer_list_->Notify(
       FROM_HERE, &FieldTrialList::Observer::OnFieldTrialGroupFinalized,
       field_trial->trial_name(), field_trial->group_name_internal());
 }
 
 // static
+void FieldTrialList::UpdateFieldTrialAllocator(FieldTrial* field_trial) {

[Alexei Svitkine (slow), 2016/10/19 16:32:48]

This should be in the anonymous namespace at the top of the file if it's not
used outside of the .cc file.

[lawrencewu, 2016/10/19 20:18:25]

This function references the |field_trial_allocator| private member so I don't
think we can do that.

[Alexei Svitkine (slow), 2016/10/19 20:39:36]

If field_trial_allocator is not used outside of the .cc file, put it into the
anon namespace too and prefix it with g_ for the naming convention.

[lawrencewu, 2016/10/20 17:36:05]

Moved.

+  // Don't do anything if the allocator hasn't been instantiated yet.
+  if (field_trial_allocator == nullptr)
+    return;
+
+  // We allocate just enough memory to fit the FTE struct, trial name, and group
+  // name. If there's no more memory available, quit early.
+  FieldTrial::State trial_state;
+  field_trial->GetState(&trial_state);
+
+  std::string trial_name;
+  trial_state.trial_name.CopyToString(&trial_name);
+  size_t trial_name_size = trial_name.size() + 1;
+
+  std::string group_name;
+  trial_state.group_name.CopyToString(&group_name);
+  size_t group_name_size = group_name.size() + 1;
+
+  uint32_t trial_name_offset = sizeof(FieldTrialEntry);
+  uint32_t group_name_offset = trial_name_offset + trial_name_size;
+  size_t size = sizeof(FieldTrialEntry) + trial_name_size + group_name_size;
+
+  base::SharedPersistentMemoryAllocator::Reference ref =
+      field_trial_allocator->Allocate(size, kFieldTrialType);
+  if (ref == base::SharedPersistentMemoryAllocator::kReferenceNull)
+    return;
+
+  char* dst = field_trial_allocator->GetAsObject<char>(ref, kFieldTrialType);
+  FieldTrialEntry fte = {trial_state.activated, group_name_offset};
+  memcpy(dst, &fte, sizeof(FieldTrialEntry));

[Alexei Svitkine (slow), 2016/10/19 16:32:49]

Just cast dst to FTE* and set it directly rather than having a temporary on the
stack that you memcpy into.

[lawrencewu, 2016/10/19 20:18:26]

Done.

+  memcpy(dst + trial_name_offset, trial_name.c_str(), trial_name_size);
+  memcpy(dst + group_name_offset, group_name.c_str(), group_name_size);
+
+  field_trial_allocator->MakeIterable(ref);
+}
+
+// static
 size_t FieldTrialList::GetFieldTrialCount() {
   if (!global_)
     return 0;