Store field trial parameters 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 0720f12c06906d2096f8055e2acada85dbd877fd..db14e03bc0f11b2046b55dc980a7b04b10608578 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -13,6 +13,7 @@
 #include "base/debug/alias.h"
 #include "base/feature_list.h"
 #include "base/logging.h"
+#include "base/metrics/field_trial_param_associator.h"
 #include "base/pickle.h"
 #include "base/process/memory.h"
 #include "base/rand_util.h"
@@ -68,23 +69,54 @@ struct FieldTrialEntry {
   // Size of the pickled structure, NOT the total size of this entry.
   uint32_t size;
 
-  // Calling this is only valid when the entry is initialized. That is, it
-  // resides in shared memory and has a pickle containing the trial name and
-  // group name following it.
-  bool GetTrialAndGroupName(StringPiece* trial_name,
-                            StringPiece* group_name) const {
+  // Returns an iterator over the data containing names and params.
+  PickleIterator GetPickleIterator() const {
     char* src = reinterpret_cast<char*>(const_cast<FieldTrialEntry*>(this)) +
                 sizeof(FieldTrialEntry);
 
     Pickle pickle(src, size);
-    PickleIterator pickle_iter(pickle);
+    return PickleIterator(pickle);
+  }
 
-    if (!pickle_iter.ReadStringPiece(trial_name))
+  // Takes the iterator and writes out the first two items into |trial_name| and
+  // |group_name|.
+  bool ReadStringPair(PickleIterator iter,
+                      StringPiece* trial_name,
+                      StringPiece* group_name) const {
+    if (!iter.ReadStringPiece(trial_name))
       return false;
-    if (!pickle_iter.ReadStringPiece(group_name))
+    if (!iter.ReadStringPiece(group_name))
       return false;
     return true;
   }
+
+  // Calling this is only valid when the entry is initialized. That is, it
+  // resides in shared memory and has a pickle containing the trial name and
+  // group name following it.
+  bool GetTrialAndGroupName(StringPiece* trial_name,
+                            StringPiece* group_name) const {
+    return ReadStringPair(GetPickleIterator(), trial_name, group_name);
+  }
+
+  // Calling this is only valid when the entry is initialized as well. Reads the
+  // parameters following the trial and group name and stores them as key-value
+  // mappings in |params|.
+  bool GetParams(std::map<std::string, std::string>* params) const {
+    PickleIterator iter = GetPickleIterator();
+    StringPiece tmp;
+    if (!ReadStringPair(iter, &tmp, &tmp))

[Alexei Svitkine (slow), 2016/11/22 17:46:37]

You're passing iter by value - which means it gets copied and its position being
advance by ReadStringPair() doesn't modify iter in this function.

Which means you'll end up having the first param/value pair end up being the
field trial name and group name.

You can fix this by passing iter by pointer. But please also add test coverage
for this - i.e. check the size of new_params map in your test to make sure it's
as expected.

[lawrencewu, 2016/11/22 20:25:44]

Done.

+      return false;
+
+    while (true) {
+      StringPiece key;
+      StringPiece value;
+      if (!iter.ReadStringPiece(&key))
+        return true;  // No more params to read.
+      if (!iter.ReadStringPiece(&value))
+        return false;
+      (*params)[key.as_string()] = value.as_string();
+    }
+  }
 };
 
 // Created a time value based on |year|, |month| and |day_of_month| parameters.
@@ -193,6 +225,26 @@ HANDLE CreateReadOnlyHandle(FieldTrialList::FieldTrialAllocator* allocator) {
 }
 #endif
 
+bool PickleFieldTrial(const FieldTrial::State& trial_state, Pickle* pickle) {

[Alexei Svitkine (slow), 2016/11/22 17:46:37]

Add a comment.

[lawrencewu, 2016/11/22 20:25:44]

Done.

+  if (!pickle->WriteString(trial_state.trial_name))
+    return false;
+  if (!pickle->WriteString(trial_state.group_name))
+    return false;
+
+  // Get field trial params.
+  std::map<std::string, std::string> params;
+  FieldTrialParamAssociator::GetInstance()->GetFieldTrialParamsWithoutFallback(
+      trial_state.trial_name.as_string(), trial_state.group_name.as_string(),
+      &params);
+
+  // Write params to pickle.
+  for (const auto& param : params) {
+    pickle->WriteString(StringPiece(param.first));
+    pickle->WriteString(StringPiece(param.second));
+  }
+  return true;
+}

[Alexei Svitkine (slow), 2016/11/22 17:46:37]

Can you put this function right underneath struct FieldTrialEntry?

[lawrencewu, 2016/11/22 20:25:44]

Done.

+
 }  // namespace
 
 // statics
@@ -539,6 +591,72 @@ std::string FieldTrialList::FindFullName(const std::string& trial_name) {
 }
 
 // static
+bool FieldTrialList::GetParamsFromSharedMemory(
+    FieldTrial* field_trial,
+    std::map<std::string, std::string>* params) {
+  DCHECK(global_);
+  // If the field trial allocator is not set up yet, then we are in the browser
+  // process, which should have all the parameters. So if we got here, then we

[Alexei Svitkine (slow), 2016/11/22 17:46:37]

The first part of the comment is not the only reason why field_trial_allocator_
could be not set up. For example, shared memory impl may not be enabled. Or it's
being called too early.

If you're trying to be accurate in the comment, suggest listing the cases (you
can use a bullet list).

[lawrencewu, 2016/11/22 20:25:44]

Done.

+  // couldn't find the params in field_trial_param_associator, so it's
+  // definitely not here. Return false.
+  if (!global_->field_trial_allocator_)
+    return false;
+
+  SharedPersistentMemoryAllocator::Reference ref = field_trial->ref_;

[Alexei Svitkine (slow), 2016/11/22 17:46:37]

Nit: I'm not sure the local var is needed here - can just use field_trial->ref_
directly.

[lawrencewu, 2016/11/22 20:25:44]

Done.

+  if (!ref)

[Alexei Svitkine (slow), 2016/11/22 17:46:37]

How about a comment about when this wouldn't be set?

[lawrencewu, 2016/11/22 20:25:44]

Done.

+    return false;
+
+  const FieldTrialEntry* entry =
+      global_->field_trial_allocator_->GetAsObject<const FieldTrialEntry>(
+          ref, kFieldTrialType);
+  return entry->GetParams(params);
+}
+
+// static
+void FieldTrialList::ClearParamsFromSharedMemoryForTesting() {
+  if (!global_ || !global_->field_trial_allocator_)
+    return;
+  // To clear the params, we iterate through every item in the allocator, copy
+  // just the trial and group name into a newly-allocated segment and then clear
+  // the existing item.

[Alexei Svitkine (slow), 2016/11/22 17:46:37]

This seems like a lot of work to do. I'm not sure it's necessary - my suspicion
is that most cases that clear params for testing also destroy the FieldTrialList
object. If so, then this wouldn't be necessary. 

[lawrencewu, 2016/11/22 20:25:44]

Although that's possible, and would make this unnecessary, it would still be
incorrect to call the calling functions ClearAllParamsForTesting() or
ClearAllVariationParams() if it cleared the whole FieldTrialList object and not
just the params. IMO, we should keep this.

+  SharedPersistentMemoryAllocator* allocator =
+      global_->field_trial_allocator_.get();
+  PersistentMemoryAllocator::Iterator mem_iter(allocator);
+
+  SharedPersistentMemoryAllocator::Reference prev_ref;
+  while ((prev_ref = mem_iter.GetNextOfType(kFieldTrialType)) !=
+         SharedPersistentMemoryAllocator::kReferenceNull) {
+    // Get the existing field trial entry in shared memory.
+    const FieldTrialEntry* prev_entry =
+        allocator->GetAsObject<const FieldTrialEntry>(prev_ref,
+                                                      kFieldTrialType);
+    StringPiece trial_name;
+    StringPiece group_name;
+    prev_entry->GetTrialAndGroupName(&trial_name, &group_name);
+
+    // Write a new entry, minus the params.
+    Pickle pickle;
+    pickle.WriteString(trial_name);
+    pickle.WriteString(group_name);
+    size_t total_size = sizeof(FieldTrialEntry) + pickle.size();
+    SharedPersistentMemoryAllocator::Reference new_ref =
+        allocator->Allocate(total_size, kFieldTrialType);
+    FieldTrialEntry* new_entry =
+        allocator->GetAsObject<FieldTrialEntry>(new_ref, kFieldTrialType);
+    new_entry->activated = prev_entry->activated;
+    new_entry->size = pickle.size();
+
+    // Update the ref on the field trial.
+    FieldTrial* trial = Find(trial_name.as_string());
+    trial->ref_ = new_ref;
+
+    // Mark the existing entry as unused.
+    uint32_t oldType = allocator->GetType(prev_ref);
+    allocator->ChangeType(prev_ref, 0, oldType);
+  }
+}
+
+// static
 bool FieldTrialList::TrialExists(const std::string& trial_name) {
   return Find(trial_name) != NULL;
 }
@@ -938,8 +1056,8 @@ void FieldTrialList::AddToAllocatorWhileLocked(FieldTrial* field_trial) {
     return;
 
   Pickle pickle;
-  pickle.WriteString(trial_state.trial_name);
-  pickle.WriteString(trial_state.group_name);
+  if (!PickleFieldTrial(trial_state, &pickle))
+    return;
 
   size_t total_size = sizeof(FieldTrialEntry) + pickle.size();
   FieldTrial::FieldTrialRef ref =