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..96395f4fe26dbad71d4b2be87db31a3d5b96b0db 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -39,6 +39,20 @@ const char kActivationMarker = '*';
 const bool kUseSharedMemoryForFieldTrials = false;
 #endif
 
+const base::StringPiece kAllocatorName = "field_trial_allocator";

[Alexei Svitkine (slow), 2016/10/12 20:41:00]

const char kAllocatorName[] =

[lawrencewu, 2016/10/14 04:54:09]

Done.

+const uint64_t kAllocatorId = 1337;

[bcwhite, 2016/10/13 14:28:02]

If you don't care about this, you can always pass 0 during the initialization of
the allocator.

[lawrencewu, 2016/10/14 04:54:09]

Done.

+const uint64_t kFieldTrialType = 7331;

[bcwhite, 2016/10/13 14:28:02]

Allocator ID is 64 bits but type IDs are only 32 bits.

In case the same allocator could ever be shared by different systems, I like
generating IDs aren't likely to conflict.

echo -n "FieldTrialEntry" | sha1sum  =>  0xABA17E13

Also, I suggest adding +1 to indicate this is version #1 of the structure. 
Increment the version if the structure changes to ensure there's no possible
mismatch if somehow a new version of the code ends up reading old data.  (Not
likely given your use-case but being consistent this way also provides a
consistent example to future coders, who perhaps might encounter this.)

[lawrencewu, 2016/10/14 04:54:09]

Changed to the sha1sum + 1.

+
+// 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.

[bcwhite, 2016/10/13 14:28:02]

Document that two strings are appended to the end of this structure, both NUL
terminated.  You can make this somewhat self-documenting by exchanging the
group_name_offset by two _length fields, one per string.
You can also add a "char strings[1]" member at the end but then you have to be
careful about calculating the total length because padding will add more bytes
to sizeof(struct) that are obvious.

You can see an example of this here:
https://cs.chromium.org/chromium/src/base/metrics/persistent_histogram_alloca...

[lawrencewu, 2016/10/14 04:54:09]

I added a comment. While adding the name[1] member would be nice if we only had
one string, I think it would be a little confusing with two appended strings.

[bcwhite, 2016/10/14 13:56:56]

You could simply call it "strings" instead of "name".  I think it would make the
code clearer for future maintainers but so long as it's documented clearly, I
won't press it.

[lawrencewu, 2016/10/19 16:21:28]

Ack, will drop.

+struct FieldTrialEntry {
+  bool activated;
+  uint32_t 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) {
   DCHECK_GT(year, 1970);
@@ -132,6 +146,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,9 +593,7 @@ 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 =
@@ -587,13 +601,11 @@ void FieldTrialList::CreateTrialsFromCommandLine(
 
     // 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.

[bcwhite, 2016/10/13 14:28:02]

... only for the duration of this method.

[lawrencewu, 2016/10/14 04:54:09]

Fixed.

-    base::SharedMemory shared_memory(shm_handle, false);
-    shared_memory.Map(field_trial_length);
+    std::unique_ptr<base::SharedMemory> shm(
+        new base::SharedMemory(shm_handle, false));
+    shm.get()->Map(2 << 12);  // 4 KiB = one page

[bcwhite, 2016/10/13 14:28:02]

It looks like the comment is describing the constant but the numbers don't
match.  I think the comment is describing "<< 12" but that's not clear.

Better to use "<<10" for KiB (or "<<20" for MiB) as that's more recognizable.

  shm.get()->Map(8 << 10);  // 8KiB (1 memory page == 4KiB)

Also, is a memory page always 4KiB?  Is it important to this?

[lawrencewu, 2016/10/14 04:54:09]

Whoops, that should really just be 4KiB, so the number was off. The default
memory page size if 4KiB, this number was just chosen as a convenient threshold.

 
-    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
@@ -607,36 +619,61 @@ void FieldTrialList::CreateTrialsFromCommandLine(
 }
 
 // static
-std::unique_ptr<base::SharedMemory> FieldTrialList::CopyFieldTrialStateToFlags(
+void FieldTrialList::CreateTrialsFromSharedMemory(
+    std::unique_ptr<base::SharedMemory> shm) {
+  base::SharedPersistentMemoryAllocator shalloc(std::move(shm), kAllocatorId,
+                                                kAllocatorName, true);

[bcwhite, 2016/10/13 14:28:02]

Since you're declaring the allocator as read-only, you should GetAsObject as
const objects.

[bcwhite, 2016/10/13 18:41:15]

Actually, if you make this a "const" object then you should get a compiler error
when trying to get a non-const pointer to objects.

[lawrencewu, 2016/10/14 04:54:09]

Ack, will ignore this.

[bcwhite, 2016/10/14 13:56:56]

No, seriously.  Make it "const".  This case is exactly why I put "const" support
in the object to begin with.

[lawrencewu, 2016/10/19 16:21:28]

Oh, my bad, I misunderstood you (thought your second comment meant it couldn't
be done because of the compiler). I made the allocator const and made
GetAsObject get const FieldTrialEntries.

+  base::PersistentMemoryAllocator::Iterator iter(&shalloc);
+
+  uint32_t type;
+  base::SharedPersistentMemoryAllocator::Reference ref = iter.GetNext(&type);

[bcwhite, 2016/10/13 14:28:02]

This will return a reference of any type but below you expect it to be
kFieldTrialType.  Use iter.GetNextOfType() to fetch specific references or
switch based on the returned type.

[lawrencewu, 2016/10/14 04:54:09]

Done.

+  while (ref != base::SharedPersistentMemoryAllocator::kReferenceNull) {

[bcwhite, 2016/10/13 14:28:02]

How about:

while ((ref = iter.GetNextOfType(kFieldTrialType)) != kReferenceNull) {
  ...
}

This avoids having to do another GetNext at the bottom of the block.

[lawrencewu, 2016/10/14 04:54:09]

Very nice, done.

+    char* src = shalloc.GetAsObject<char>(ref, kFieldTrialType);
+
+    FieldTrialEntry fte;
+    memcpy(&fte, src, sizeof(FieldTrialEntry));

[Alexei Svitkine (slow), 2016/10/12 20:41:00]

This memcpy is unnecessary. You can just cast src to a FieldTrialEntry*.

[lawrencewu, 2016/10/14 04:54:09]

Done.

+
+    char* trial_name = src + sizeof(FieldTrialEntry);
+    char* group_name = src + fte.group_name_offset;

[Alexei Svitkine (slow), 2016/10/12 20:41:00]

Can you make GetTrialName() and GetGroupName() accessors on the struct so this
code doesn't need to have the logic?

[lawrencewu, 2016/10/14 04:54:09]

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();
+    }
+
+    ref = iter.GetNext(&type);
+  }
+}
+
+// static
+void FieldTrialList::CopyFieldTrialStateToFlags(
     const char* field_trial_handle_switch,
     base::CommandLine* cmd_line) {
-  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

[bcwhite, 2016/10/13 14:28:02]

Move comment inside #ifdef and fix indent.

[lawrencewu, 2016/10/14 04:54:09]

Done.

 // 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;
-    }
+  if (kUseSharedMemoryForFieldTrials) {
+    base::SharedMemory* shm = field_trial_allocator->shared_memory();
+
+    // HANDLE is just typedef'd to void *
+    auto uintptr_handle =
+        reinterpret_cast<std::uintptr_t>(shm->handle().GetHandle());

[bcwhite, 2016/10/13 14:28:02]

Is std:: required for uintptr_t?

[lawrencewu, 2016/10/14 04:54:09]

Looks like no. Omitted.

+    std::string field_trial_handle = std::to_string(uintptr_handle);
+
+    cmd_line->AppendSwitchASCII(field_trial_handle_switch, field_trial_handle);

[bcwhite, 2016/10/13 14:28:02]

So...  We can take a local void*, convert it to an int, convert that to a
string, pass it to a completely separate process, do the reverse conversions,
and still end up with a valid handle?

Impressive... and worthy of a comment (perhaps even an external reference) as to
why this works.

[lawrencewu, 2016/10/14 04:54:09]

Yup, pretty hacky...added a comment and link to the SO where I found this trick.

+    return;
+  }
 #endif
+
+  std::string field_trial_states;
+  base::FieldTrialList::AllStatesToString(&field_trial_states);
+  if (!field_trial_states.empty()) {
     cmd_line->AppendSwitchASCII(switches::kForceFieldTrials,
                                 field_trial_states);
   }
-  return std::unique_ptr<base::SharedMemory>(nullptr);
 }
 
 // static
@@ -694,12 +731,64 @@ void FieldTrialList::NotifyFieldTrialGroupSelection(FieldTrial* field_trial) {
   if (!field_trial->enable_field_trial_)
     return;
 
+#if defined(OS_WIN)

[bcwhite, 2016/10/13 14:28:02]

This isn't windows-specific code...  Can we omit the #ifdef and rely upon the
flag?

[lawrencewu, 2016/10/14 04:54:09]

Yeah, we can do that. Omitted.

+  if (kUseSharedMemoryForFieldTrials) {
+    FieldTrialList::UpdateFieldTrialAllocator(field_trial);
+  }
+#endif
+
   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) {
+  // Create the allocator if not already created.
+  if (field_trial_allocator == nullptr) {

[bcwhite, 2016/10/13 14:28:02]

Are all calls to this method guaranteed to be on the same thread?  If not, you
have a race condition creating the allocator.  After that, operations (below) on
the allocator are thread-safe.

[lawrencewu, 2016/10/14 04:54:09]

I'm not too sure about this. I'll do some investigation, but first, @asvitkine,
do you have any insight on this?

[lawrencewu, 2016/10/19 16:21:28]

Turns out that it is not guaranteed to be thread-safe, so I wrapped the
instantiation of the allocator in a lock.

+    std::unique_ptr<base::SharedMemory> shm(new base::SharedMemory());
+    shm->CreateAndMapAnonymous(2 << 12);  // 4 KiB = one page
+
+    field_trial_allocator = new base::SharedPersistentMemoryAllocator(
+        std::move(shm), kAllocatorId, kAllocatorName, false);
+  }
+
+  // We allocate just enough memory to fit the FTE struct, which contains
+  // whether or not the field trial is activated and the offset from the start
+  // of the struct to the group name.
+  // By the end of this function, the piece of allocated memory will look like
+  // this:
+  // ---------------------------------
+  // | fte | trial_name | group_name |
+  // ---------------------------------
+  // with fte pointing  ^
+  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);
+  field_trial_allocator->MakeIterable(ref);

[bcwhite, 2016/10/13 14:28:02]

Move this to the end.  Don't make it iterable until after it has been filled.

[lawrencewu, 2016/10/14 04:54:09]

Done.

+
+  char* dst = field_trial_allocator->GetAsObject<char>(ref, kFieldTrialType);
+  FieldTrialEntry fte = {trial_state.activated, group_name_offset};
+  memcpy(dst, &fte, sizeof(FieldTrialEntry));
+  memcpy(dst + trial_name_offset, trial_name.c_str(), trial_name_size);
+  memcpy(dst + group_name_offset, group_name.c_str(), group_name_size);
+}
+
+// static
 size_t FieldTrialList::GetFieldTrialCount() {
   if (!global_)
     return 0;