Support add-on 'field trial' records.

components/metrics/persistent_system_profile.cc

 // Copyright 2017 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 "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 {
 
 namespace {
 
 // To provide atomic addition of records so that there is no confusion between
 // writers and readers, all of the metadata about a record is contained in a
 // structure that can be stored as a single atomic 32-bit word.
 union RecordHeader {
   struct {
     unsigned continued : 1;  // Flag indicating if there is more after this.
     unsigned type : 7;       // The type of this record.
     unsigned amount : 24;    // The amount of data to follow.
   } as_parts;
   base::subtle::Atomic32 as_atomic;
 };
 
 constexpr uint32_t kTypeIdSystemProfile = 0x330A7150;  // SHA1(SystemProfile)
 constexpr size_t kSystemProfileAllocSize = 4 << 10;    // 4 KiB
 constexpr size_t kMaxRecordSize = (1 << 24) - sizeof(RecordHeader);
 
 static_assert(sizeof(RecordHeader) == sizeof(base::subtle::Atomic32),
               "bad RecordHeader size");
 
 // Calculate the size of a record based on the amount of data. This adds room
 // for the record header and rounds up to the next multiple of the record-header
 // size.
 size_t CalculateRecordSize(size_t data_amount) {
   return (data_amount + sizeof(RecordHeader) + sizeof(RecordHeader) - 1) &
          ~(sizeof(RecordHeader) - 1);

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

I recently noticed we have base::Align():

https://cs.chromium.org/chromium/src/base/bits.h?rcl=b3bc42922ee5b49c6b44cf71...

Can we use that here?

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

Done.

 }
 
 }  // namespace
 
 PersistentSystemProfile::RecordAllocator::RecordAllocator(
     base::PersistentMemoryAllocator* memory_allocator,
     size_t min_size)
     : allocator_(memory_allocator),
       has_complete_profile_(false),
       alloc_reference_(0),
@@ skipping 24 matching lines @@
     base::subtle::NoBarrier_Store(&header->as_atomic, 0);
   }
 
   // Reset member variables.
   has_complete_profile_ = false;
   alloc_reference_ = 0;
   alloc_size_ = 0;
   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();
 
   // Allocate space and write records until everything has been stored.
   do {
     if (end_offset_ == alloc_size_) {
       if (!AddSegment(remaining_size))
         return false;
     }
     // Write out as much of the data as possible. |data| and |remaining_size|
@@ skipping 67 matching lines @@
   allocator_->MakeIterable(ref);
 
   alloc_reference_ = ref;
   alloc_size_ = allocator_->GetAllocSize(ref);
   return true;
 }
 
 bool PersistentSystemProfile::RecordAllocator::WriteData(
     RecordType type,
     const char** data,
     size_t* remaining_size) {

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

Reading the code again, I think it would be clearer for this to be named
data_size.

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

Done.

   char* block =
       allocator_->GetAsArray<char>(alloc_reference_, kTypeIdSystemProfile,
                                    base::PersistentMemoryAllocator::kSizeAny);
   if (!block)
     return false;  // It's bad if there is no accessible block.
 
   size_t write_size = std::min(*remaining_size, kMaxRecordSize);
   write_size =
       std::min(write_size, alloc_size_ - end_offset_ - sizeof(RecordHeader));
 
   // Write the data and the record header.
   RecordHeader header;
   header.as_atomic = 0;
   header.as_parts.type = type;
   header.as_parts.amount = write_size;
   header.as_parts.continued = (write_size < *remaining_size);
   size_t offset = end_offset_;
   end_offset_ += CalculateRecordSize(write_size);
   DCHECK_GE(alloc_size_, end_offset_);
   if (end_offset_ < alloc_size_) {
     // An empty record header has to be next before this one gets written.
     base::subtle::NoBarrier_Store(
         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.
   *data += write_size;
   *remaining_size -= write_size;
 
   return true;
 }
 
 bool PersistentSystemProfile::RecordAllocator::ReadData(
@@ skipping 19 matching lines @@
     return true;  // End of all records.
   } else if (*type == kUnusedSpace) {
     *type = static_cast<RecordType>(header.as_parts.type);
   } else if (*type != header.as_parts.type) {
     NOTREACHED();  // Continuation didn't match start of record.
     *type = kUnusedSpace;
     record->clear();
     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_);
 
   return !continued;
 }
 
 PersistentSystemProfile::PersistentSystemProfile() {}
 
 PersistentSystemProfile::~PersistentSystemProfile() {}
 
@@ skipping 24 matching lines @@
     bool complete) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
   if (allocators_.empty() || serialized_profile.empty())
     return;
 
   for (auto& allocator : allocators_) {
     // 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.
     if (complete)
       allocator.set_complete_profile();
   }
 
   if (complete)
     all_have_complete_profile_ = true;
 }
 
 void PersistentSystemProfile::SetSystemProfile(
     const SystemProfileProto& profile,
     bool complete) {
   // Avoid serialization if passed profile is not complete and all allocators
   // already have complete ones.
   if (!complete && all_have_complete_profile_)
     return;
 
   std::string serialized_profile;
   if (!profile.SerializeToString(&serialized_profile))
     return;
   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) {
   const RecordAllocator records(&memory_allocator);
   return records.HasMoreData();
 }
 
 // static
 bool PersistentSystemProfile::GetSystemProfile(
     const base::PersistentMemoryAllocator& memory_allocator,
     SystemProfileProto* system_profile) {
   const RecordAllocator records(&memory_allocator);
 
   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() {
   return base::Singleton<
       GlobalPersistentSystemProfile,
       base::LeakySingletonTraits<GlobalPersistentSystemProfile>>::get();
 }
 
 }  // namespace metrics