Support add-on 'field trial' records.

components/metrics/persistent_system_profile.cc

Index: components/metrics/persistent_system_profile.cc
diff --git a/components/metrics/persistent_system_profile.cc b/components/metrics/persistent_system_profile.cc
index 88b2a76bb257365096a06b86d0113837d1597007..4063bf3f2fb1be3a6f267db3fe0558e100ee3fce 100644
--- a/components/metrics/persistent_system_profile.cc
+++ b/components/metrics/persistent_system_profile.cc
@@ -4,10 +4,14 @@
 
 #include "components/metrics/persistent_system_profile.h"
 
+#include <set>
+
 #include "base/atomicops.h"
 #include "base/memory/singleton.h"
 #include "base/metrics/persistent_memory_allocator.h"
+#include "base/pickle.h"
 #include "base/stl_util.h"
+#include "components/variations/active_field_trials.h"
 
 namespace metrics {
 
@@ -82,9 +86,8 @@ void PersistentSystemProfile::RecordAllocator::Reset() {
   end_offset_ = 0;
 }
 
-bool PersistentSystemProfile::RecordAllocator::Write(
-    RecordType type,
-    const std::string& record) {
+bool PersistentSystemProfile::RecordAllocator::Write(RecordType type,
+                                                     base::StringPiece record) {
   const char* data = record.data();
   size_t remaining_size = record.size();
 
@@ -198,8 +201,9 @@ bool PersistentSystemProfile::RecordAllocator::WriteData(
         reinterpret_cast<base::subtle::Atomic32*>(block + end_offset_), 0);
   }
   memcpy(block + offset + sizeof(header), *data, write_size);
-  base::subtle::Release_Store(reinterpret_cast<base::subtle::Atomic32*>(block),
-                              header.as_atomic);
+  base::subtle::Release_Store(
+      reinterpret_cast<base::subtle::Atomic32*>(block + offset),

[Alexei Svitkine (slow), 2017/06/28 14:59:00]

Wait, so this is now writing the header at block+offset instead of at the start
of the block? What about the existing header that was there at the start of the
block?

Also, I don't follow how this interacts with the comment on line 199.

Let me try to reason about it and you can tell me if I'm missing something:

So we have a block. Which may already have a header & data from a previous write
and thus end_offset_ may be non-0.

We now want to write more data to it. Previous code would overwrite the old
header but this change inserts a new header instead. In terms of size
calculation, CalculateRecordSize() adds extra room for the header beyond
write_size.*

So I guess it was just a bug with the previous logic?

But how does line 199 logic fit in? If the end_offset is within the alloc then
we also write a blank header? What about if it's greater than alloc size? Seems
strange we don't have handling for that...

*Although I now find it quite confusing because we're calculating size on lines
185-187 and line 196 and reasoning about their relationship is pretty difficult.
How about moving both calculations to one place and doing something like:

const size_t max_write_size = std::min(kMaxRecordSize, alloc_size_ - end_offset_
- sizeof(RecordHeader));
size_t write_size = std::min(*remaining_size, max_write_size);
size_t record_size = CalculateRecordSize(write_size);
DCHECK_LE(record_size, max_write_size);

I think that should keep the current logic the same and make it a bit more
readable.

[bcwhite, 2017/06/28 16:43:30]

It was an error where each additional record had its header written at the start
of the block instead at the proper location.  Previous tests only wrote one
record.
Concurrency requires an zero end-of-records header.  Before the current "zero"
header is overwritten with a new one, the location of the next header (after the
record being written) must be cleared.  Line 200 writes to block+end_offset_
(after the being-written record) while this writes to block+offset(the address
of the being-written record).

Because the memory may be used, there's no guarantee that it's zeroed.
Yes.
When memory is reset, the first word of every block is zeroed but the rest of
the space is untouched.
Done.

+      header.as_atomic);
 
   // Account for what was stored and prepare for follow-on records with any
   // remaining data.
@@ -239,15 +243,14 @@ bool PersistentSystemProfile::RecordAllocator::ReadData(
     return false;
   }
   size_t read_size = header.as_parts.amount;
-  if (read_size < sizeof(header) ||
-      end_offset_ + sizeof(header) + read_size > alloc_size_) {
+  if (end_offset_ + sizeof(header) + read_size > alloc_size_) {
     NOTREACHED();  // Invalid header amount.
     *type = kUnusedSpace;
     return true;  // Don't try again.
   }
 
   // Append the record data to the output string.
-  record->append(block + sizeof(header), read_size);
+  record->append(block + end_offset_ + sizeof(header), read_size);
   end_offset_ += CalculateRecordSize(read_size);
   DCHECK_GE(alloc_size_, end_offset_);
 
@@ -292,8 +295,10 @@ void PersistentSystemProfile::SetSystemProfile(
     // Don't overwrite a complete profile with an incomplete one.
     if (!complete && allocator.has_complete_profile())
       continue;
-    // A full system profile always starts fresh.
-    allocator.Reset();
+    // A full system profile always starts fresh. Incomplete keeps existing
+    // records for merging.
+    if (complete)
+      allocator.Reset();
     // Write out the serialized profile.
     allocator.Write(kSystemProfileProto, serialized_profile);
     // Indicate if this is a complete profile.
@@ -319,6 +324,22 @@ void PersistentSystemProfile::SetSystemProfile(
   SetSystemProfile(serialized_profile, complete);
 }
 
+void PersistentSystemProfile::SetFieldTrial(base::StringPiece trial,
+                                            base::StringPiece group) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
+  if (trial.empty() || group.empty())

[Alexei Svitkine (slow), 2017/06/28 14:59:00]

This shouldn't be possible. Can these be DCHECKs instead?

[bcwhite, 2017/06/28 16:43:30]

Done.

+    return;
+
+  base::Pickle pickler;
+  if (!pickler.WriteString(trial) || !pickler.WriteString(group))
+    return;
+
+  WriteToAll(kFieldTrialInfo,
+             base::StringPiece(static_cast<const char*>(pickler.data()),
+                               pickler.size()));
+}
+
 // static
 bool PersistentSystemProfile::HasSystemProfile(
     const base::PersistentMemoryAllocator& memory_allocator) {
@@ -334,12 +355,73 @@ bool PersistentSystemProfile::GetSystemProfile(
 
   RecordType type;
   std::string record;
-  if (!records.Read(&type, &record))
-    return false;
-  if (type != kSystemProfileProto)
+  do {
+    if (!records.Read(&type, &record))
+      return false;
+  } while (type != kSystemProfileProto);
+
+  if (!system_profile->ParseFromString(record))
     return false;
 
-  return system_profile->ParseFromString(record);
+  MergeUpdateRecords(memory_allocator, system_profile);
+  return true;
+}
+
+// static
+void PersistentSystemProfile::MergeUpdateRecords(
+    const base::PersistentMemoryAllocator& memory_allocator,
+    SystemProfileProto* system_profile) {
+  const RecordAllocator records(&memory_allocator);
+
+  RecordType type;
+  std::string record;
+  std::set<uint32_t> known_field_trial_ids;
+
+  while (records.Read(&type, &record)) {

[Alexei Svitkine (slow), 2017/06/28 14:59:01]

Maybe add a comment about why this can't be done in the same record iteration in
GetSystemProfile(). (i.e. presumably because you want to get the full profile
first before messing with field trials.)

[bcwhite, 2017/06/28 16:43:31]

Mostly it just made a nice, independent block of code.  But I'm also aware that
we may one day detect that a "complete" profile doesn't actually contain
everything and it'll be necessary to merge update records into new profiles. 
That, should it become necessary, would re-use this method.

+    switch (type) {
+      case kUnusedSpace:
+        // These should never be returned.
+        NOTREACHED();
+        break;
+
+      case kSystemProfileProto:
+        // Profile was passed in; ignore this one.
+        break;
+
+      case kFieldTrialInfo: {
+        // Get the set of known trial IDs so duplicates don't get added.
+        if (known_field_trial_ids.empty()) {
+          for (int i = 0; i < system_profile->field_trial_size(); ++i) {
+            const SystemProfileProto::FieldTrial& field_trial =
+                system_profile->field_trial(i);

[Alexei Svitkine (slow), 2017/06/28 14:59:00]

Nit: Inline this into the line below. 

[bcwhite, 2017/06/28 16:43:31]

Done.

+            known_field_trial_ids.insert(field_trial.name_id());
+          }
+        }
+
+        base::Pickle pickler(record.data(), record.size());
+        base::PickleIterator iter(pickler);
+        base::StringPiece trial;
+        base::StringPiece group;
+        if (iter.ReadStringPiece(&trial) && iter.ReadStringPiece(&group)) {
+          variations::ActiveGroupId field_ids =
+              variations::MakeActiveGroupId(trial, group);

[Alexei Svitkine (slow), 2017/06/28 14:59:00]

Hmm, maybe we can just store ActiveGroupId in the first place in the data rather
than strings? Would use less memory.

[bcwhite, 2017/06/28 16:43:31]

Yes.  However, that means calculating the two SHA1 hashes for the strings which
is relatively expensive.  If it's done at "read" time, it will probably never
have to be done at all because either a new "complete" system profile will
overwrite the string records or the file will be removed by a clean shutdown.

For these reasons, it seems better to store the strings quickly into the memory
block that is already reserved than spend cpu cycles converting them to numbers.

+          if (!base::ContainsKey(known_field_trial_ids, field_ids.name)) {
+            SystemProfileProto::FieldTrial* field_trial =
+                system_profile->add_field_trial();
+            field_trial->set_name_id(field_ids.name);
+            field_trial->set_group_id(field_ids.group);
+            known_field_trial_ids.insert(field_ids.name);
+          }
+        }
+      } break;
+    }
+  }
+}
+
+void PersistentSystemProfile::WriteToAll(RecordType type,
+                                         base::StringPiece record) {
+  for (auto& allocator : allocators_)
+    allocator.Write(type, record);
 }
 
 GlobalPersistentSystemProfile* GlobalPersistentSystemProfile::GetInstance() {