Use pickle for field trial entries in shared memory

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 bb6ae0cb83fd02fa52b8d17584ee911fcef105ca..120158798fcfb43a8ab1b73ac8323b585fb82212 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -13,6 +13,7 @@
 #include "base/feature_list.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/pickle.h"
 #include "base/process/memory.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
@@ -46,30 +47,16 @@ const uint32_t kFieldTrialType = 0xABA17E13 + 1;  // SHA1(FieldTrialEntry) v1
 const size_t kFieldTrialAllocationSize = 4 << 10;  // 4 KiB = one page
 #endif
 
-// 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 |
-// ---------------------------------
+// We create one FieldTrialEntry per field trial in shared memory, via
+// AddToAllocatorWhileLocked. It contains whether or not it's activated, and a
+// base::Pickle object that we unpickle and read from.
 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;
-  }
+  // Size of the pickled structure, NOT the total size of this entry.
+  uint32_t size;
+  // Space for this will be added during allocation. This is really variable
+  // length but we can't have flexible arrays in structs.
+  char data[1];
 };
 
 // Created a time value based on |year|, |month| and |day_of_month| parameters.
@@ -798,15 +785,24 @@ void FieldTrialList::CreateTrialsFromSharedMemory(
     std::unique_ptr<SharedMemory> shm) {
   const SharedPersistentMemoryAllocator shalloc(std::move(shm), 0,
                                                 kAllocatorName, true);
-  PersistentMemoryAllocator::Iterator iter(&shalloc);
+  PersistentMemoryAllocator::Iterator mem_iter(&shalloc);
 
   SharedPersistentMemoryAllocator::Reference ref;
-  while ((ref = iter.GetNextOfType(kFieldTrialType)) !=
+  while ((ref = mem_iter.GetNextOfType(kFieldTrialType)) !=
          SharedPersistentMemoryAllocator::kReferenceNull) {
     const FieldTrialEntry* entry =
         shalloc.GetAsObject<const FieldTrialEntry>(ref, kFieldTrialType);
+    Pickle pickle(entry->data, entry->size);
+    PickleIterator pickle_iter(pickle);
+
+    StringPiece trial_name;
+    StringPiece group_name;
+    if (!pickle_iter.ReadStringPiece(&trial_name))
+      continue;

[Alexei Svitkine (slow), 2016/10/27 21:36:50]

I'm not a fan of this error handling. Can you make it NOTREACHED() at least?

Also, how about making a helper function on FieldTrialEntry for this? e.g. it
can return a bool and have out params for the trial name and group name.

[lawrencewu, 2016/10/28 14:23:03]

Done.

+    if (!pickle_iter.ReadStringPiece(&group_name))
+      continue;
     FieldTrial* trial =
-        CreateFieldTrial(entry->GetTrialName(), entry->GetGroupName());
+        CreateFieldTrial(trial_name.as_string(), group_name.as_string());

[Alexei Svitkine (slow), 2016/10/27 21:05:02]

Can you add a TODO above the statement to convert the APIs to take StringPieces
so we don't have to do extra copies here?

[lawrencewu, 2016/10/27 21:24:19]

Done.

 
     if (entry->activated) {
       // Call |group()| to mark the trial as "used" and notify observers, if
@@ -866,34 +862,21 @@ void FieldTrialList::AddToAllocatorWhileLocked(FieldTrial* field_trial) {
   if (!field_trial->GetState(&trial_state))
     return;
 
-  size_t trial_name_size = trial_state.trial_name.size() + 1;
-  size_t group_name_size = trial_state.group_name.size() + 1;
-  size_t size = sizeof(FieldTrialEntry) + trial_name_size + group_name_size;
+  Pickle pickle;
+  pickle.WriteString(trial_state.trial_name);
+  pickle.WriteString(trial_state.group_name);
 
-  // Allocate just enough memory to fit the FieldTrialEntry struct, trial name,
-  // and group name.
+  size_t total_size = sizeof(bool) + +sizeof(uint32_t) + pickle.size();

[Alexei Svitkine (slow), 2016/10/27 21:05:02]

You have an extra + in front of sizeof.

[lawrencewu, 2016/10/27 21:24:19]

Fixed.

[Alexei Svitkine (slow), 2016/10/27 21:36:49]

This size calculation seems error prone.

How about just not having the data[1] field and just using
sizeof(FieldTrialEntry) + pickle.size()?

[lawrencewu, 2016/10/28 14:23:03]

That sounds better, done.

[Alexei Svitkine (slow), 2016/10/28 15:25:29]

I don't see the change - did you forget to upload a patchset?

   SharedPersistentMemoryAllocator::Reference ref =
-      allocator->Allocate(size, kFieldTrialType);
+      allocator->Allocate(total_size, kFieldTrialType);
   if (ref == SharedPersistentMemoryAllocator::kReferenceNull)
     return;
 
-  // Calculate offsets into the shared memory segment.
   FieldTrialEntry* entry =
       allocator->GetAsObject<FieldTrialEntry>(ref, kFieldTrialType);
-  uint32_t trial_name_offset = sizeof(FieldTrialEntry);
-  uint32_t group_name_offset = trial_name_offset + trial_name_size;
-
-  // Copy over the data.
   entry->activated = trial_state.activated;
-  entry->group_name_offset = group_name_offset;
-  char* trial_name = reinterpret_cast<char*>(entry) + trial_name_offset;
-  char* group_name = reinterpret_cast<char*>(entry) + group_name_offset;
-  memcpy(trial_name, trial_state.trial_name.data(), trial_name_size);
-  memcpy(group_name, trial_state.group_name.data(), group_name_size);
-
-  // Null terminate the strings.
-  trial_name[trial_state.trial_name.size()] = '\0';
-  group_name[trial_state.group_name.size()] = '\0';
+  entry->size = pickle.size();
+  memcpy(entry->data, pickle.data(), pickle.size());

[Alexei Svitkine (slow), 2016/10/27 21:05:02]

This is doing an extra copy. Let's avoid that - since the data size will grow
with parameters.

You can use PickleSizer to calculate the size and allocate the memory for that,
then create the Pickle over the actual memory.

[lawrencewu, 2016/10/27 21:24:19]

I'm not sure we should do this. PickleSizer returns the size excluding header
length, which is private to Pickle. There's also no way to easily create a
writable Pickle over the actual memory. I think we should stick with getting the
size from the actual pickle itself and just copying it over.

[Alexei Svitkine (slow), 2016/10/27 21:36:49]

PickleSizer specifically documents that it's meant to provide an accurate
representation of a Pickle's size - so it's guaranteed to match. If they don't
match it would be a bug in one of those two classes.

However, your other point appears to be true - that Pickle doesn't currently
support a use case that doesn't copy memory. This is unfortunate. Ideally, we
could expand it to support that use case - but I'd rather we don't detract from
the main project for that for now - it can always be added later. So how about
adding a TODO about it here and we can revisit it if there's time in your
internship - or else it could be done afterward.

[lawrencewu, 2016/10/28 14:23:03]

The docs say that PickleSizer accurately computes the size of a Pickle's
_payload_, which I think is different from a Pickle's actual data size (which
includes, by default, a header of size sizeof(uint32_t)). That said, I see no
reason why PickleSizer shouldn't also be able to compute a Pickle's total size,
so I could possibly tackle that afterwards too.

As for the other point, done.

 
   allocator->MakeIterable(ref);
   field_trial->ref_ = ref;