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
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
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 abou
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
|
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 - si
lawrencewu
2016/10/27 21:24:19
I'm not sure we should do this. PickleSizer return
Alexei Svitkine (slow)
2016/10/27 21:36:49
PickleSizer specifically documents that it's meant
lawrencewu
2016/10/28 14:23:03
The docs say that PickleSizer accurately computes
|
allocator->MakeIterable(ref); |
field_trial->ref_ = ref; |