Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2449783007:
Use pickle for field trial entries in shared memory (Closed)
|
Created:
4 years, 1 month ago by lawrencewu Modified:
4 years, 1 month ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse pickle for field trial entries in shared memory
This CL changes the data format we use to store field trials in shared
memory from custom offsets to a pickled structure, which has several
advantages. Namely, it handles offsets for us and requires less code.
We will also probably end up using this for experiment parameters as
well.
BUG=660128
Committed: https://crrev.com/ab1da09492fa8e84ae6d90151bd718efdf97d3f4
Cr-Commit-Position: refs/heads/master@{#428423}
Patch Set 1 #Patch Set 2 : turn flag back to false #
Total comments: 13
Patch Set 3 : address comments #Patch Set 4 : address comments #
Total comments: 9
Patch Set 5 : address comments #
Total comments: 2
Patch Set 6 : address nits #Messages
Total messages: 19 (6 generated)
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lawrencewu@chromium.org changed reviewers: + asvitkine@chromium.org
I'll add parameter support in the next CL, but this should be a decent start. I'm thinking of just writing each parameter key and value after the trial name and group name, and then when we want to get the parameters, we can just iterate over the pickle, ignoring the first two entries, and look for the parameter with our key.
https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:805: CreateFieldTrial(trial_name.as_string(), group_name.as_string()); Can you add a TODO above the statement to convert the APIs to take StringPieces so we don't have to do extra copies here? https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:869: size_t total_size = sizeof(bool) + +sizeof(uint32_t) + pickle.size(); You have an extra + in front of sizeof. https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:879: memcpy(entry->data, pickle.data(), pickle.size()); This is doing an extra copy. Let's avoid that - since the data size will grow with parameters. You can use PickleSizer to calculate the size and allocate the memory for that, then create the Pickle over the actual memory.
Address comments. https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:805: CreateFieldTrial(trial_name.as_string(), group_name.as_string()); On 2016/10/27 21:05:02, Alexei Svitkine (slow) wrote: > Can you add a TODO above the statement to convert the APIs to take StringPieces > so we don't have to do extra copies here? Done. https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:869: size_t total_size = sizeof(bool) + +sizeof(uint32_t) + pickle.size(); On 2016/10/27 21:05:02, Alexei Svitkine (slow) wrote: > You have an extra + in front of sizeof. Fixed. https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:879: memcpy(entry->data, pickle.data(), pickle.size()); On 2016/10/27 21:05:02, Alexei Svitkine (slow) wrote: > This is doing an extra copy. Let's avoid that - since the data size will grow > with parameters. > > You can use PickleSizer to calculate the size and allocate the memory for that, > then create the Pickle over the actual memory. I'm not sure we should do this. PickleSizer returns the size excluding header length, which is private to Pickle. There's also no way to easily create a writable Pickle over the actual memory. I think we should stick with getting the size from the actual pickle itself and just copying it over.
https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:801: continue; I'm not a fan of this error handling. Can you make it NOTREACHED() at least? Also, how about making a helper function on FieldTrialEntry for this? e.g. it can return a bool and have out params for the trial name and group name. https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:869: size_t total_size = sizeof(bool) + +sizeof(uint32_t) + pickle.size(); This size calculation seems error prone. How about just not having the data[1] field and just using sizeof(FieldTrialEntry) + pickle.size()? https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:879: memcpy(entry->data, pickle.data(), pickle.size()); On 2016/10/27 21:24:19, lawrencewu wrote: > On 2016/10/27 21:05:02, Alexei Svitkine (slow) wrote: > > This is doing an extra copy. Let's avoid that - since the data size will grow > > with parameters. > > > > You can use PickleSizer to calculate the size and allocate the memory for > that, > > then create the Pickle over the actual memory. > > I'm not sure we should do this. PickleSizer returns the size excluding header > length, which is private to Pickle. There's also no way to easily create a > writable Pickle over the actual memory. I think we should stick with getting the > size from the actual pickle itself and just copying it over. PickleSizer specifically documents that it's meant to provide an accurate representation of a Pickle's size - so it's guaranteed to match. If they don't match it would be a bug in one of those two classes. However, your other point appears to be true - that Pickle doesn't currently support a use case that doesn't copy memory. This is unfortunate. Ideally, we could expand it to support that use case - but I'd rather we don't detract from the main project for that for now - it can always be added later. So how about adding a TODO about it here and we can revisit it if there's time in your internship - or else it could be done afterward.
Address comments. https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:801: continue; On 2016/10/27 21:36:50, Alexei Svitkine (slow) wrote: > I'm not a fan of this error handling. Can you make it NOTREACHED() at least? > > Also, how about making a helper function on FieldTrialEntry for this? e.g. it > can return a bool and have out params for the trial name and group name. Done. https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:869: size_t total_size = sizeof(bool) + +sizeof(uint32_t) + pickle.size(); On 2016/10/27 21:36:49, Alexei Svitkine (slow) wrote: > This size calculation seems error prone. > > How about just not having the data[1] field and just using > sizeof(FieldTrialEntry) + pickle.size()? That sounds better, done. https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:879: memcpy(entry->data, pickle.data(), pickle.size()); On 2016/10/27 21:36:49, Alexei Svitkine (slow) wrote: > On 2016/10/27 21:24:19, lawrencewu wrote: > > On 2016/10/27 21:05:02, Alexei Svitkine (slow) wrote: > > > This is doing an extra copy. Let's avoid that - since the data size will > grow > > > with parameters. > > > > > > You can use PickleSizer to calculate the size and allocate the memory for > > that, > > > then create the Pickle over the actual memory. > > > > I'm not sure we should do this. PickleSizer returns the size excluding header > > length, which is private to Pickle. There's also no way to easily create a > > writable Pickle over the actual memory. I think we should stick with getting > the > > size from the actual pickle itself and just copying it over. > > PickleSizer specifically documents that it's meant to provide an accurate > representation of a Pickle's size - so it's guaranteed to match. If they don't > match it would be a bug in one of those two classes. > > However, your other point appears to be true - that Pickle doesn't currently > support a use case that doesn't copy memory. This is unfortunate. Ideally, we > could expand it to support that use case - but I'd rather we don't detract from > the main project for that for now - it can always be added later. So how about > adding a TODO about it here and we can revisit it if there's time in your > internship - or else it could be done afterward. The docs say that PickleSizer accurately computes the size of a Pickle's _payload_, which I think is different from a Pickle's actual data size (which includes, by default, a header of size sizeof(uint32_t)). That said, I see no reason why PickleSizer shouldn't also be able to compute a Pickle's total size, so I could possibly tackle that afterwards too. As for the other point, done.
https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:869: size_t total_size = sizeof(bool) + +sizeof(uint32_t) + pickle.size(); On 2016/10/28 14:23:03, lawrencewu wrote: > On 2016/10/27 21:36:49, Alexei Svitkine (slow) wrote: > > This size calculation seems error prone. > > > > How about just not having the data[1] field and just using > > sizeof(FieldTrialEntry) + pickle.size()? > > That sounds better, done. I don't see the change - did you forget to upload a patchset?
On 2016/10/28 15:25:29, Alexei Svitkine (slow) wrote: > https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... > File base/metrics/field_trial.cc (right): > > https://codereview.chromium.org/2449783007/diff/20001/base/metrics/field_tria... > base/metrics/field_trial.cc:869: size_t total_size = sizeof(bool) + > +sizeof(uint32_t) + pickle.size(); > On 2016/10/28 14:23:03, lawrencewu wrote: > > On 2016/10/27 21:36:49, Alexei Svitkine (slow) wrote: > > > This size calculation seems error prone. > > > > > > How about just not having the data[1] field and just using > > > sizeof(FieldTrialEntry) + pickle.size()? > > > > That sounds better, done. > > I don't see the change - did you forget to upload a patchset? Yes I did, my bad.
https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:58: bool GetTrialAndGroupName(StringPiece* trial_name, Please add a comment, specifically mentioning that this is only valid to do when the entry resides in shared memory with name/group data following it. https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:63: Pickle pickle(src, this->size); Nit: No this-> https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:811: NOTREACHED(); You can still do a continue after the NOTREACHED just in case. https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:813: // TODO: Convert the API for CreateFieldTrial to take StringPieces. TODOs should have a username or crbug here. So you can make this a TODO(lawrencewu): https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:890: // TODO: Modify base::Pickle to be able to write over a section in memory, so Same comment about the TODO.
Address comments. https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:63: Pickle pickle(src, this->size); On 2016/10/28 15:32:22, Alexei Svitkine (slow) wrote: > Nit: No this-> Done. https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:811: NOTREACHED(); On 2016/10/28 15:32:22, Alexei Svitkine (slow) wrote: > You can still do a continue after the NOTREACHED just in case. Done. https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:813: // TODO: Convert the API for CreateFieldTrial to take StringPieces. On 2016/10/28 15:32:22, Alexei Svitkine (slow) wrote: > TODOs should have a username or crbug here. > > So you can make this a TODO(lawrencewu): Done. https://codereview.chromium.org/2449783007/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:890: // TODO: Modify base::Pickle to be able to write over a section in memory, so On 2016/10/28 15:32:22, Alexei Svitkine (slow) wrote: > Same comment about the TODO. Done.
lgtm https://codereview.chromium.org/2449783007/diff/80001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449783007/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:52: // base::Pickle object that we unpickle and read from. Nit: Remove "It contains whether or not it's activated." and instead put a comment above the member. Keep the comment about the pickle but expand to make it clear that the Pickle follos the FieldTrialEntry struct in memory. https://codereview.chromium.org/2449783007/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:55: // Size of the pickled structure, NOT the total size of this entry. Nit: Add a new line above the comment.
The CQ bit was checked by lawrencewu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2449783007/#ps100001 (title: "address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Use pickle for field trial entries in shared memory This CL changes the data format we use to store field trials in shared memory from custom offsets to a pickled structure, which has several advantages. Namely, it handles offsets for us and requires less code. We will also probably end up using this for experiment parameters as well. BUG=660128 ========== to ========== Use pickle for field trial entries in shared memory This CL changes the data format we use to store field trials in shared memory from custom offsets to a pickled structure, which has several advantages. Namely, it handles offsets for us and requires less code. We will also probably end up using this for experiment parameters as well. BUG=660128 Committed: https://crrev.com/ab1da09492fa8e84ae6d90151bd718efdf97d3f4 Cr-Commit-Position: refs/heads/master@{#428423} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ab1da09492fa8e84ae6d90151bd718efdf97d3f4 Cr-Commit-Position: refs/heads/master@{#428423} |
