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..3b7a9e7c48d674407b3f9b8c5907bcaf975458c0 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -12,6 +12,7 @@
 #include "base/command_line.h"
 #include "base/feature_list.h"
 #include "base/logging.h"
+#include "base/process/memory.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ -35,9 +36,37 @@ 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;
+
+  const char* GetTrialName() const {
+    const char* src =
+        reinterpret_cast<char*>(const_cast<FieldTrialEntry*>(this));
+    return src + sizeof(FieldTrialEntry);
+  }
+
+  const char* GetGroupName() const {
+    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) {
@@ -124,11 +153,23 @@ bool ParseFieldTrialsString(const std::string& trials_string,
   return true;
 }
 
+#if defined(OS_WIN)
+HANDLE CreateReadOnlyHandle(base::SharedPersistentMemoryAllocator* allocator) {
+  HANDLE dst;

[Alexei Svitkine (slow), 2016/10/21 20:52:21]

Nit: Move this right before it's needed i.e. above DuplicateHandle()

[lawrencewu, 2016/10/23 20:01:56]

Done.

+  HANDLE src = allocator->shared_memory()->handle().GetHandle();
+  ProcessHandle process = GetCurrentProcess();
+  DWORD access = SECTION_MAP_READ | SECTION_QUERY;
+  ::DuplicateHandle(process, src, process, &dst, access, true, 0);

[Alexei Svitkine (slow), 2016/10/21 20:52:21]

This function can potentially fail - as indicated by the return value.

Let's fall back to the old command line path in that case for now.

(We can revisit later.)

[lawrencewu, 2016/10/23 20:01:56]

Done.

+  return dst;
+}
+#endif
+
 }  // namespace
 
 // statics
 const int FieldTrial::kNotFinalized = -1;
 const int FieldTrial::kDefaultGroupNumber = 0;
+const int kFieldTrialAllocationSize = 4 << 10;  // 4 KiB = one page
 bool FieldTrial::enable_benchmarking_ = false;
 
 int FieldTrialList::kNoExpirationYear = 0;
@@ -332,6 +373,11 @@ FieldTrialList::FieldTrialList(
   Time::Exploded exploded;
   two_years_from_build_time.LocalExplode(&exploded);
   kNoExpirationYear = exploded.year;
+
+  field_trial_allocator_ = nullptr;
+#if defined(OS_WIN)
+  readonly_allocator_handle_ = 0;
+#endif
 }
 
 FieldTrialList::~FieldTrialList() {
@@ -586,14 +632,13 @@ void FieldTrialList::CreateTrialsFromCommandLine(
         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, true));
+    if (!shm.get()->Map(field_trial_length))
+      base::TerminateBecauseOutOfMemory(field_trial_length);
 
-    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 +651,72 @@ void FieldTrialList::CreateTrialsFromCommandLine(
   }
 }
 
+#if defined(OS_WIN)
+// static
+void FieldTrialList::AppendFieldTrialHandleIfNeeded(
+    base::HandlesToInheritVector* handles) {
+  FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded();

[Alexei Svitkine (slow), 2016/10/24 14:40:27]

Wait, what happened to this line?

Are you removing it because CopyFieldTrialStateToFlag() gets called first? If
so, you're putting a requirement on the caller to do that which might break in
the future.

I suggest either making the APIs resilient to either order - or to document the
expected order thoroughly - i.e. both in API comments in the header and also
above the call to this function from the place it's called.

[lawrencewu, 2016/10/24 15:35:13]

Added the line back. For some reason I thought I wouldn't be able to implement
the fallback behavior without removing this line, but now I realize that the
handle would still be null, since this function is idempotent.

+
+  // Need to get a new, read-only handle to share to the child.
+  handles->push_back(global_->readonly_allocator_handle_);
+}
+#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* entry =
+        shalloc.GetAsObject<const FieldTrialEntry>(ref, kFieldTrialType);
+    FieldTrial* trial =
+        CreateFieldTrial(entry->GetTrialName(), entry->GetGroupName());
+
+    if (entry->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
-std::unique_ptr<base::SharedMemory> FieldTrialList::CopyFieldTrialStateToFlags(
+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) {
+    FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded();
+    // 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>(global_->readonly_allocator_handle_);
+    size_t field_trial_length =
+        global_->field_trial_allocator_->shared_memory()->mapped_size();
+    std::string field_trial_handle = std::to_string(uintptr_handle) + "," +
+                                     std::to_string(field_trial_length);
+
+    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
@@ -689,10 +769,14 @@ void FieldTrialList::NotifyFieldTrialGroupSelection(FieldTrial* field_trial) {
     if (field_trial->group_reported_)
       return;
     field_trial->group_reported_ = true;
-  }
 
-  if (!field_trial->enable_field_trial_)
-    return;
+    if (!field_trial->enable_field_trial_)
+      return;
+
+    if (kUseSharedMemoryForFieldTrials)

[Alexei Svitkine (slow), 2016/10/21 20:52:21]

Nit: {} since now it's more than one line.

[lawrencewu, 2016/10/23 20:01:56]

Done.

[Alexei Svitkine (slow), 2016/10/24 14:40:27]

Doesn't look done to me.

[lawrencewu, 2016/10/24 15:35:13]

Woops, actually done now.

+      field_trial->AddToAllocatorWhileLocked(
+          global_->field_trial_allocator_.get());
+  }
 
   global_->observer_list_->Notify(
       FROM_HERE, &FieldTrialList::Observer::OnFieldTrialGroupFinalized,
@@ -700,6 +784,78 @@ void FieldTrialList::NotifyFieldTrialGroupSelection(FieldTrial* field_trial) {
 }
 
 // static
+void FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded() {
+  AutoLock auto_lock(global_->lock_);
+  // Create the allocator if not already created and add all existing trials.
+  if (global_->field_trial_allocator_ != nullptr)
+    return;
+
+  std::unique_ptr<base::SharedMemory> shm(new base::SharedMemory());
+  if (!shm->CreateAndMapAnonymous(kFieldTrialAllocationSize))
+    base::TerminateBecauseOutOfMemory(kFieldTrialAllocationSize);
+
+  // TODO(lawrencewu): call UpdateTrackingHistograms() when all field trials
+  // have been registered (perhaps in the destructor?)
+  global_->field_trial_allocator_.reset(
+      new base::SharedPersistentMemoryAllocator(std::move(shm), 0,
+                                                kAllocatorName, false));
+  global_->field_trial_allocator_->CreateTrackingHistograms(kAllocatorName);
+
+  // Add all existing field trials.
+  for (const auto& registered : global_->registered_) {
+    registered.second->AddToAllocatorWhileLocked(
+        global_->field_trial_allocator_.get());
+  }
+
+#if defined(OS_WIN)
+  // Set |readonly_allocator_handle_| so we can pass it to be inherited and via
+  // the command line.
+  global_->readonly_allocator_handle_ =
+      CreateReadOnlyHandle(global_->field_trial_allocator_.get());
+#endif
+}
+
+void FieldTrial::AddToAllocatorWhileLocked(
+    base::SharedPersistentMemoryAllocator* allocator) {
+  // Don't do anything if the allocator hasn't been instantiated yet.
+  if (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;

[Alexei Svitkine (slow), 2016/10/21 20:52:21]

Nit: Do you ned FieldTrial:: here?

[lawrencewu, 2016/10/23 20:01:56]

Nope, removed.

+  this->GetState(&trial_state);

[Alexei Svitkine (slow), 2016/10/21 20:52:21]

Nit: No need for this->

[lawrencewu, 2016/10/23 20:01:56]

Done.

+
+  std::string trial_name;

[Alexei Svitkine (slow), 2016/10/21 20:52:21]

I don't think these string copies here are necessary.

You should be able to copy directly from the string piece to the entry.

Now, the StringPiece is not guaranteed to have the null terminator, but that's
fine because you can just reserve the extra byte (which you already account for)
and set it to 0 explicitly (unless the memory is already guaranteed to be
0-initialized in which case it's not necessary).

[lawrencewu, 2016/10/23 20:01:56]

Done. The memory is guaranteed to be 0-initialized, at least on Windows: "The
initial contents of the pages in a file mapping object backed by the operating
system paging file are 0 (zero)." from
https://msdn.microsoft.com/en-us/library/windows/desktop/aa366537(v=vs.85).aspx

as well as:
https://cs.chromium.org/chromium/src/base/metrics/persistent_memory_allocator...

I believe that on Linux, since we use mkstemp to create the temporary file used
as backing for the shared memory, that should be zeroed out as well. On Mac,
which uses mach_vm_map(), the memory is also zero-filled:
https://books.google.ca/books?id=K8vUkpOXhN4C&pg=PA883&lpg=PA883&dq=mach_vm_m...

I am not completely sure about Linux and Mac, though -- maybe erikchen@ knows
something we don't.

[Alexei Svitkine (slow), 2016/10/24 14:40:27]

Let's just explicitly 0 the null terminator characters, if you're not entirely
sure. It's super cheap to do and this way guarantees there's no issues.

[lawrencewu, 2016/10/24 15:35:13]

Sure, done.

+  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 =
+      allocator->Allocate(size, kFieldTrialType);
+  if (ref == base::SharedPersistentMemoryAllocator::kReferenceNull)
+    return;
+
+  FieldTrialEntry* dst =
+      allocator->GetAsObject<FieldTrialEntry>(ref, kFieldTrialType);
+  FieldTrialEntry entry = {trial_state.activated, group_name_offset};
+  *dst = entry;

[Alexei Svitkine (slow), 2016/10/21 20:52:21]

Nit:

Instead of the extra copy, how about:

FieldTrialEntry* entry = allocator->GetAsObject<FieldTrialEntry>(...);
entry->activated = trial_state.activated;
entry->group_name_offset = group_name_offset;

This way, you don't create an extra struct on the stack and copy into out - and
also there's less variables in this function which makes the code simpler.

[lawrencewu, 2016/10/23 20:01:56]

Yup that's definitely cleaner. Done.

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