Store field trial parameters in shared memory

base/metrics/field_trial.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/metrics/field_trial.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/base_switches.h"
 #include "base/build_time.h"
 #include "base/command_line.h"
 #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"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 
 namespace base {
 
@@ skipping 35 matching lines @@
 // object that we unpickle and read from. Any changes to this structure requires
 // a bump in kFieldTrialType id defined above.
 struct FieldTrialEntry {
   // Whether or not this field trial is activated. This is really just a boolean
   // but marked as a uint32_t for portability reasons.
   uint32_t activated;
 
   // Size of the pickled structure, NOT the total size of this entry.
   uint32_t size;
 
+  // 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);
+    return PickleIterator(pickle);
+  }
+
+  // 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 (!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 {
-    char* src = reinterpret_cast<char*>(const_cast<FieldTrialEntry*>(this)) +
-                sizeof(FieldTrialEntry);
+    return ReadStringPair(GetPickleIterator(), trial_name, group_name);
+  }
 
-    Pickle pickle(src, size);
-    PickleIterator pickle_iter(pickle);
+  // 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;
 
-    if (!pickle_iter.ReadStringPiece(trial_name))
-      return false;
-    if (!pickle_iter.ReadStringPiece(group_name))
-      return false;
-    return true;
+    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.
 Time CreateTimeFromParams(int year, int month, int day_of_month) {
   DCHECK_GT(year, 1970);
   DCHECK_GT(month, 0);
   DCHECK_LT(month, 13);
   DCHECK_GT(day_of_month, 0);
   DCHECK_LT(day_of_month, 32);
@@ skipping 89 matching lines @@
   HANDLE src = allocator->shared_memory()->handle().GetHandle();
   ProcessHandle process = GetCurrentProcess();
   DWORD access = SECTION_MAP_READ | SECTION_QUERY;
   HANDLE dst;
   if (!::DuplicateHandle(process, src, process, &dst, access, true, 0))
     return nullptr;
   return dst;
 }
 #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
 const int FieldTrial::kNotFinalized = -1;
 const int FieldTrial::kDefaultGroupNumber = 0;
 bool FieldTrial::enable_benchmarking_ = false;
 
 int FieldTrialList::kNoExpirationYear = 0;
 
 //------------------------------------------------------------------------------
@@ skipping 326 matching lines @@
 
 // static
 std::string FieldTrialList::FindFullName(const std::string& trial_name) {
   FieldTrial* field_trial = Find(trial_name);
   if (field_trial)
     return field_trial->group_name();
   return std::string();
 }
 
 // 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;
 }
 
 // static
 bool FieldTrialList::IsTrialActive(const std::string& trial_name) {
   FieldTrial* field_trial = Find(trial_name);
   FieldTrial::ActiveGroup active_group;
   return field_trial && field_trial->GetActiveGroup(&active_group);
 }
@@ skipping 379 matching lines @@
   FieldTrial::State trial_state;
   if (!field_trial->GetStateWhileLocked(&trial_state))
     return;
 
   // Or if we've already added it. We must check after GetState since it can
   // also add to the allocator.
   if (field_trial->ref_)
     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 =
       allocator->Allocate(total_size, kFieldTrialType);
   if (ref == FieldTrialAllocator::kReferenceNull)
     return;
 
   FieldTrialEntry* entry =
       allocator->GetAsObject<FieldTrialEntry>(ref, kFieldTrialType);
   entry->activated = trial_state.activated;
@@ skipping 57 matching lines @@
     return;
   }
   AutoLock auto_lock(global_->lock_);
   CHECK(!global_->PreLockedFind(trial->trial_name())) << trial->trial_name();
   trial->AddRef();
   trial->SetTrialRegistered();
   global_->registered_[trial->trial_name()] = trial;
 }
 
 }  // namespace base