Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2412113002:
Use SharedPersistentMemoryAllocator to share field trial state (Closed)
|
Created:
4 years, 2 months ago by lawrencewu Modified:
4 years, 1 month ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, piman+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse SharedPersistentMemoryAllocator to share field trial state
Change the method by which we share field trial state from using a
SharedMemory class to SharedPersistentMemoryAllocator. Adds this
allocator to the base::FieldTrialList singleton, so there is only one copy
of this state on the browser process vs. a copy for each process host
which is how it currently works (from
https://codereview.chromium.org/2365273004/)
BUG=653874
Committed: https://crrev.com/0b496492defaf5f2f70e08318c36568ccce9ce87
Cr-Commit-Position: refs/heads/master@{#427378}
Patch Set 1 #Patch Set 2 : git pull + merge #Patch Set 3 : add some documentation #
Total comments: 53
Patch Set 4 : address alexei's comments #Patch Set 5 : address bcwhite comments #
Total comments: 12
Patch Set 6 : address comments #
Total comments: 22
Patch Set 7 : address comments #Patch Set 8 : set boolean back to false #Patch Set 9 : add some comments #Patch Set 10 : address comments #Patch Set 11 : move field_trial_allocator into anon namespace #
Total comments: 16
Patch Set 12 : address comments #Patch Set 13 : add lock #
Total comments: 22
Patch Set 14 : address comments #
Total comments: 22
Patch Set 15 : fix some nits #Patch Set 16 : address comments #
Total comments: 8
Patch Set 17 : address comments #Patch Set 18 : set null terminator #
Total comments: 6
Patch Set 19 : address nits #Patch Set 20 : fix unittest #Patch Set 21 : hide instantiating allocator behind flag #Patch Set 22 : move include back to .cc file #
Total comments: 6
Patch Set 23 : address comments and please compiler #Patch Set 24 : gclient sync #Messages
Total messages: 83 (47 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Use SharedPersistentMemoryAllocator to share field trial state between processes instead of passing it via a string in shared memory. Adds the allocator to the base::FieldTrialList singleton. BUG=653874 ========== to ========== Use SharedPersistentMemoryAllocator to share field trial state between processes instead of passing it via a string in shared memory. Adds the allocator to the base::FieldTrialList singleton. BUG=653874 ==========
lawrencewu@chromium.org changed reviewers: + asvitkine@chromium.org, bcwhite@chromium.org
@bcwhite: This is the CL I mentioned to you about using the SharedPersistentMemoryAllocator. @all: Still need to write some tests, but would you mind taking a look at this CL now?
https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:42: const base::StringPiece kAllocatorName = "field_trial_allocator"; const char kAllocatorName[] = https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:634: memcpy(&fte, src, sizeof(FieldTrialEntry)); This memcpy is unnecessary. You can just cast src to a FieldTrialEntry*. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:637: char* group_name = src + fte.group_name_offset; Can you make GetTrialName() and GetGroupName() accessors on the struct so this code doesn't need to have the logic? https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.h:491: std::unique_ptr<base::SharedMemory> shm); If you're only calling this from the .cc file, it should be in the anon namespace in that file. Unless it needs to reference private functions/fields of the class, in which case you can keep it a static member, but make it private. https://codereview.chromium.org/2412113002/diff/40001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/2412113002/diff/40001/content/browser/child_p... content/browser/child_process_launcher.cc:156: if (base::FieldTrialList::field_trial_allocator) { Instead of this construct, how about making a function on FieldTrialList that takes |handles| and appends it if necessary. Then, it's a simple call here and you don't need to expose the allocator object outside the .cc file.
https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:43: const uint64_t kAllocatorId = 1337; If you don't care about this, you can always pass 0 during the initialization of the allocator. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:44: const uint64_t kFieldTrialType = 7331; Allocator ID is 64 bits but type IDs are only 32 bits. In case the same allocator could ever be shared by different systems, I like generating IDs aren't likely to conflict. echo -n "FieldTrialEntry" | sha1sum => 0xABA17E13 Also, I suggest adding +1 to indicate this is version #1 of the structure. Increment the version if the structure changes to ensure there's no possible mismatch if somehow a new version of the code ends up reading old data. (Not likely given your use-case but being consistent this way also provides a consistent example to future coders, who perhaps might encounter this.) https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:50: // from the start of the struct. Document that two strings are appended to the end of this structure, both NUL terminated. You can make this somewhat self-documenting by exchanging the group_name_offset by two _length fields, one per string. You can also add a "char strings[1]" member at the end but then you have to be careful about calculating the total length because padding will add more bytes to sizeof(struct) that are obvious. You can see an example of this here: https://cs.chromium.org/chromium/src/base/metrics/persistent_histogram_alloca... https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:603: // only for the duration of this call currently anyway. ... only for the duration of this method. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:606: shm.get()->Map(2 << 12); // 4 KiB = one page It looks like the comment is describing the constant but the numbers don't match. I think the comment is describing "<< 12" but that's not clear. Better to use "<<10" for KiB (or "<<20" for MiB) as that's more recognizable. shm.get()->Map(8 << 10); // 8KiB (1 memory page == 4KiB) Also, is a memory page always 4KiB? Is it important to this? https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:625: kAllocatorName, true); Since you're declaring the allocator as read-only, you should GetAsObject as const objects. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:629: base::SharedPersistentMemoryAllocator::Reference ref = iter.GetNext(&type); This will return a reference of any type but below you expect it to be kFieldTrialType. Use iter.GetNextOfType() to fetch specific references or switch based on the returned type. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:630: while (ref != base::SharedPersistentMemoryAllocator::kReferenceNull) { How about: while ((ref = iter.GetNextOfType(kFieldTrialType)) != kReferenceNull) { ... } This avoids having to do another GetNext at the bottom of the block. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:655: // Use shared memory to pass the state if the feature is enabled, otherwise Move comment inside #ifdef and fix indent. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:663: reinterpret_cast<std::uintptr_t>(shm->handle().GetHandle()); Is std:: required for uintptr_t? https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:666: cmd_line->AppendSwitchASCII(field_trial_handle_switch, field_trial_handle); So... We can take a local void*, convert it to an int, convert that to a string, pass it to a completely separate process, do the reverse conversions, and still end up with a valid handle? Impressive... and worthy of a comment (perhaps even an external reference) as to why this works. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:734: #if defined(OS_WIN) This isn't windows-specific code... Can we omit the #ifdef and rely upon the flag? https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:748: if (field_trial_allocator == nullptr) { Are all calls to this method guaranteed to be on the same thread? If not, you have a race condition creating the allocator. After that, operations (below) on the allocator are thread-safe. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:782: field_trial_allocator->MakeIterable(ref); Move this to the end. Don't make it iterable until after it has been filled. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.h:329: static base::SharedPersistentMemoryAllocator* field_trial_allocator; I don't suppose there is a more generic place to hold this? It could be "child_process_info_allocator" or something, the idea being that it could be then (re-)used if/when other information comes along that would benefit from being passed this way. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.h:523: static void UpdateFieldTrialAllocator(FieldTrial* field_trial); Your comment says "adds" but the name says "update". https://codereview.chromium.org/2412113002/diff/40001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/2412113002/diff/40001/content/browser/child_p... content/browser/child_process_launcher.cc:158: base::FieldTrialList::field_trial_allocator->shared_memory() The shared memory MUST be passed read-only. Renderers are untrusted and we can't allow for a compromised one to be able to alter these settings. Ensure that is the case and document it.
https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:625: kAllocatorName, true); On 2016/10/13 14:28:02, bcwhite wrote: > Since you're declaring the allocator as read-only, you should GetAsObject as > const objects. Actually, if you make this a "const" object then you should get a compiler error when trying to get a non-const pointer to objects.
Address most comments, still need to do some investigation into making sure the shared memory is read-only, and whether there's a race condition in creating the allocator. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:42: const base::StringPiece kAllocatorName = "field_trial_allocator"; On 2016/10/12 20:41:00, Alexei Svitkine (slow) wrote: > const char kAllocatorName[] = > Done. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:43: const uint64_t kAllocatorId = 1337; On 2016/10/13 14:28:02, bcwhite wrote: > If you don't care about this, you can always pass 0 during the initialization of > the allocator. Done. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:44: const uint64_t kFieldTrialType = 7331; On 2016/10/13 14:28:02, bcwhite wrote: > Allocator ID is 64 bits but type IDs are only 32 bits. > > In case the same allocator could ever be shared by different systems, I like > generating IDs aren't likely to conflict. > > echo -n "FieldTrialEntry" | sha1sum => 0xABA17E13 > > Also, I suggest adding +1 to indicate this is version #1 of the structure. > Increment the version if the structure changes to ensure there's no possible > mismatch if somehow a new version of the code ends up reading old data. (Not > likely given your use-case but being consistent this way also provides a > consistent example to future coders, who perhaps might encounter this.) Changed to the sha1sum + 1. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:50: // from the start of the struct. On 2016/10/13 14:28:02, bcwhite wrote: > Document that two strings are appended to the end of this structure, both NUL > terminated. You can make this somewhat self-documenting by exchanging the > group_name_offset by two _length fields, one per string. > You can also add a "char strings[1]" member at the end but then you have to be > careful about calculating the total length because padding will add more bytes > to sizeof(struct) that are obvious. > > You can see an example of this here: > https://cs.chromium.org/chromium/src/base/metrics/persistent_histogram_alloca... I added a comment. While adding the name[1] member would be nice if we only had one string, I think it would be a little confusing with two appended strings. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:603: // only for the duration of this call currently anyway. On 2016/10/13 14:28:02, bcwhite wrote: > ... only for the duration of this method. Fixed. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:606: shm.get()->Map(2 << 12); // 4 KiB = one page On 2016/10/13 14:28:02, bcwhite wrote: > It looks like the comment is describing the constant but the numbers don't > match. I think the comment is describing "<< 12" but that's not clear. > > Better to use "<<10" for KiB (or "<<20" for MiB) as that's more recognizable. > > shm.get()->Map(8 << 10); // 8KiB (1 memory page == 4KiB) > > Also, is a memory page always 4KiB? Is it important to this? Whoops, that should really just be 4KiB, so the number was off. The default memory page size if 4KiB, this number was just chosen as a convenient threshold. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:625: kAllocatorName, true); On 2016/10/13 18:41:15, bcwhite wrote: > On 2016/10/13 14:28:02, bcwhite wrote: > > Since you're declaring the allocator as read-only, you should GetAsObject as > > const objects. > > Actually, if you make this a "const" object then you should get a compiler error > when trying to get a non-const pointer to objects. Ack, will ignore this. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:629: base::SharedPersistentMemoryAllocator::Reference ref = iter.GetNext(&type); On 2016/10/13 14:28:02, bcwhite wrote: > This will return a reference of any type but below you expect it to be > kFieldTrialType. Use iter.GetNextOfType() to fetch specific references or > switch based on the returned type. Done. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:630: while (ref != base::SharedPersistentMemoryAllocator::kReferenceNull) { On 2016/10/13 14:28:02, bcwhite wrote: > How about: > > while ((ref = iter.GetNextOfType(kFieldTrialType)) != kReferenceNull) { > ... > } > > This avoids having to do another GetNext at the bottom of the block. Very nice, done. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:634: memcpy(&fte, src, sizeof(FieldTrialEntry)); On 2016/10/12 20:41:00, Alexei Svitkine (slow) wrote: > This memcpy is unnecessary. You can just cast src to a FieldTrialEntry*. Done. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:637: char* group_name = src + fte.group_name_offset; On 2016/10/12 20:41:00, Alexei Svitkine (slow) wrote: > Can you make GetTrialName() and GetGroupName() accessors on the struct so this > code doesn't need to have the logic? Done. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:655: // Use shared memory to pass the state if the feature is enabled, otherwise On 2016/10/13 14:28:02, bcwhite wrote: > Move comment inside #ifdef and fix indent. Done. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:663: reinterpret_cast<std::uintptr_t>(shm->handle().GetHandle()); On 2016/10/13 14:28:02, bcwhite wrote: > Is std:: required for uintptr_t? Looks like no. Omitted. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:666: cmd_line->AppendSwitchASCII(field_trial_handle_switch, field_trial_handle); On 2016/10/13 14:28:02, bcwhite wrote: > So... We can take a local void*, convert it to an int, convert that to a > string, pass it to a completely separate process, do the reverse conversions, > and still end up with a valid handle? > > Impressive... and worthy of a comment (perhaps even an external reference) as to > why this works. Yup, pretty hacky...added a comment and link to the SO where I found this trick. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:734: #if defined(OS_WIN) On 2016/10/13 14:28:02, bcwhite wrote: > This isn't windows-specific code... Can we omit the #ifdef and rely upon the > flag? Yeah, we can do that. Omitted. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:748: if (field_trial_allocator == nullptr) { On 2016/10/13 14:28:02, bcwhite wrote: > Are all calls to this method guaranteed to be on the same thread? If not, you > have a race condition creating the allocator. After that, operations (below) on > the allocator are thread-safe. I'm not too sure about this. I'll do some investigation, but first, @asvitkine, do you have any insight on this? https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:782: field_trial_allocator->MakeIterable(ref); On 2016/10/13 14:28:02, bcwhite wrote: > Move this to the end. Don't make it iterable until after it has been filled. Done. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.h:329: static base::SharedPersistentMemoryAllocator* field_trial_allocator; On 2016/10/13 14:28:02, bcwhite wrote: > I don't suppose there is a more generic place to hold this? It could be > "child_process_info_allocator" or something, the idea being that it could be > then (re-)used if/when other information comes along that would benefit from > being passed this way. While that would be nice, I'm reluctant to do that now since I don't see an immediate need for that yet and it would probably require extra effort to hoist this variable up into a more appropriate class. (This CL itself is already kind of an effort to hoist the shared memory from gpu_process_host.cc and render_process_host_impl.cc) https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.h:491: std::unique_ptr<base::SharedMemory> shm); On 2016/10/12 20:41:00, Alexei Svitkine (slow) wrote: > If you're only calling this from the .cc file, it should be in the anon > namespace in that file. Unless it needs to reference private functions/fields of > the class, in which case you can keep it a static member, but make it private. It needs to reference regstration_ when creating the field trial, so I made it private instead. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.h:523: static void UpdateFieldTrialAllocator(FieldTrial* field_trial); On 2016/10/13 14:28:02, bcwhite wrote: > Your comment says "adds" but the name says "update". It is kind of an "update the list of field trials by adding this new one", but I changed the comment to more clearly reflect that. https://codereview.chromium.org/2412113002/diff/40001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/2412113002/diff/40001/content/browser/child_p... content/browser/child_process_launcher.cc:156: if (base::FieldTrialList::field_trial_allocator) { On 2016/10/12 20:41:00, Alexei Svitkine (slow) wrote: > Instead of this construct, how about making a function on FieldTrialList that > takes |handles| and appends it if necessary. Then, it's a simple call here and > you don't need to expose the allocator object outside the .cc file. Done. Created a AppendFieldTrialHandleIfNeeded() function and made the allocator private. https://codereview.chromium.org/2412113002/diff/40001/content/browser/child_p... content/browser/child_process_launcher.cc:158: base::FieldTrialList::field_trial_allocator->shared_memory() On 2016/10/13 14:28:02, bcwhite wrote: > The shared memory MUST be passed read-only. Renderers are untrusted and we > can't allow for a compromised one to be able to alter these settings. Ensure > that is the case and document it. If I understand the shared memory model correctly, this means that if the shared memory is NOT read-only (which, I believe, is the case), then I must duplicate the handle with read-only permissions and pass that handle down, right? Haven't done any work on this issue yet, just want to clarify.
FYI: It's considered good practice to upload a new version of your code that contains all the review changes _before_ publishing your reply comments. This allows reviewers to check how you've put into practice the thoughts going back and forth to ensure correct communication. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:50: // from the start of the struct. On 2016/10/14 04:54:09, lawrencewu wrote: > On 2016/10/13 14:28:02, bcwhite wrote: > > Document that two strings are appended to the end of this structure, both NUL > > terminated. You can make this somewhat self-documenting by exchanging the > > group_name_offset by two _length fields, one per string. > > You can also add a "char strings[1]" member at the end but then you have to be > > careful about calculating the total length because padding will add more bytes > > to sizeof(struct) that are obvious. > > > > You can see an example of this here: > > > https://cs.chromium.org/chromium/src/base/metrics/persistent_histogram_alloca... > > I added a comment. While adding the name[1] member would be nice if we only had > one string, I think it would be a little confusing with two appended strings. You could simply call it "strings" instead of "name". I think it would make the code clearer for future maintainers but so long as it's documented clearly, I won't press it. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:625: kAllocatorName, true); On 2016/10/14 04:54:09, lawrencewu wrote: > On 2016/10/13 18:41:15, bcwhite wrote: > > On 2016/10/13 14:28:02, bcwhite wrote: > > > Since you're declaring the allocator as read-only, you should GetAsObject as > > > const objects. > > > > Actually, if you make this a "const" object then you should get a compiler > error > > when trying to get a non-const pointer to objects. > > Ack, will ignore this. No, seriously. Make it "const". This case is exactly why I put "const" support in the object to begin with. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.h:329: static base::SharedPersistentMemoryAllocator* field_trial_allocator; On 2016/10/14 04:54:10, lawrencewu wrote: > On 2016/10/13 14:28:02, bcwhite wrote: > > I don't suppose there is a more generic place to hold this? It could be > > "child_process_info_allocator" or something, the idea being that it could be > > then (re-)used if/when other information comes along that would benefit from > > being passed this way. > > While that would be nice, I'm reluctant to do that now since I don't see an > immediate need for that yet and it would probably require extra effort to hoist > this variable up into a more appropriate class. (This CL itself is already kind > of an effort to hoist the shared memory from gpu_process_host.cc and > render_process_host_impl.cc) That's fine. Add a comment saying that such _could_ be done if extra data passing is desired. And keep it in mind as you develop as sometimes there are choices that would make such a future effort easier. https://codereview.chromium.org/2412113002/diff/40001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/2412113002/diff/40001/content/browser/child_p... content/browser/child_process_launcher.cc:158: base::FieldTrialList::field_trial_allocator->shared_memory() On 2016/10/14 04:54:10, lawrencewu wrote: > On 2016/10/13 14:28:02, bcwhite wrote: > > The shared memory MUST be passed read-only. Renderers are untrusted and we > > can't allow for a compromised one to be able to alter these settings. Ensure > > that is the case and document it. > > If I understand the shared memory model correctly, this means that if the shared > memory is NOT read-only (which, I believe, is the case), then I must duplicate > the handle with read-only permissions and pass that handle down, right? > > Haven't done any work on this issue yet, just want to clarify. Sounds reasonable but I don't know. You'll have to ask somebody with more low-level windows experience.
> FYI: It's considered good practice to upload a new version of your code that Oops! Scratch this. My screen just didn't show me the updated patch. Sorry.
https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:42: const uint32_t kFieldTrialType = 0xABA17E13 + 1; // Version #1 of type // SHA1(FieldTrialEntry) v1 So that others know where this number comes from. https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:772: std::move(shm), 0, kAllocatorName, false); If you call allocator->CreateTrackingHistograms(kAllocatorName); You'll get two histograms added to UMA that report the number of allocations and the amount of memory used. See existing UMA.PersistentAllocator.* entries in tools/metrics/histograms/histograms.xml. The catch is that the "UsedPct" entry is updated only with a manual call so at some point after all the field trials have been updated, you need to call allocator->UpdateTrackingHistograms(); With this you'll be able to tell if your memory segment is large enough once its out in the field. Keep the maximum utilization under 75% and you'll be okay. https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:793: field_trial_allocator->Allocate(size, kFieldTrialType); You need to handle the failure case here. If the field trial list exceeds the memory size, this will return a null reference which will become nullptr down below. As it stands, Chrome will crash if there are too many field trials to store -- an accident waiting to happen.
https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:58: char* GetTrialName(FieldTrialEntry* fte) { Make this actually a member of FieldTrialEntry - i.e. a struct can have methods just like a class. Then, you don't need |fte| parameter, because you can use |this|. https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:59: char* src = (char*)fte; Per the style guide, you must use C++ style casts rather than C casts. https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:756: FieldTrialList::UpdateFieldTrialAllocator(field_trial); Please make this lazy. That is, someone could use base/ and even the FieldTrial API without ever needing to create a child process or give it experiment state. For those use cases, we don't want to create the shared memory eagerly because it's wasteful. Instead, make the first call to AppendFieldTrialHandleIfNeeded() trigger the creation of it originally and then thereafter append to it if it's been created already.
Based on my reading of base/process/launch_win.cc, passing in a named HANDLE with lowered permissions should be sufficient. That being said, I'd feel a lot better if you tested this functionality to ensure this is the case. See https://cs.chromium.org/chromium/src/base/memory/shared_memory_win_unittest.c... for an example of a cross-process handle-sharing permissions test. You'll want to check that you can't raise the permissions of the HANDLE in the destination process.
On 2016/10/18 19:57:28, erikchen wrote: > Based on my reading of base/process/launch_win.cc, passing in a named HANDLE > with lowered permissions should be sufficient. That being said, I'd feel a lot > better if you tested this functionality to ensure this is the case. See > https://cs.chromium.org/chromium/src/base/memory/shared_memory_win_unittest.c... > for an example of a cross-process handle-sharing permissions test. > > You'll want to check that you can't raise the permissions of the HANDLE in the > destination process. Isn't there already a test for this here? https://cs.chromium.org/chromium/src/base/memory/shared_memory_win_unittest.c...
On 2016/10/18 19:57:28, erikchen wrote: > Based on my reading of base/process/launch_win.cc, passing in a named HANDLE > with lowered permissions should be sufficient. That being said, I'd feel a lot > better if you tested this functionality to ensure this is the case. See > https://cs.chromium.org/chromium/src/base/memory/shared_memory_win_unittest.c... > for an example of a cross-process handle-sharing permissions test. > > You'll want to check that you can't raise the permissions of the HANDLE in the > destination process. Isn't there already a test for this here? https://cs.chromium.org/chromium/src/base/memory/shared_memory_win_unittest.c...
Addressed asvitkine and bcwhite's comments. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:50: // from the start of the struct. On 2016/10/14 13:56:56, bcwhite wrote: > On 2016/10/14 04:54:09, lawrencewu wrote: > > On 2016/10/13 14:28:02, bcwhite wrote: > > > Document that two strings are appended to the end of this structure, both > NUL > > > terminated. You can make this somewhat self-documenting by exchanging the > > > group_name_offset by two _length fields, one per string. > > > You can also add a "char strings[1]" member at the end but then you have to > be > > > careful about calculating the total length because padding will add more > bytes > > > to sizeof(struct) that are obvious. > > > > > > You can see an example of this here: > > > > > > https://cs.chromium.org/chromium/src/base/metrics/persistent_histogram_alloca... > > > > I added a comment. While adding the name[1] member would be nice if we only > had > > one string, I think it would be a little confusing with two appended strings. > > You could simply call it "strings" instead of "name". I think it would make the > code clearer for future maintainers but so long as it's documented clearly, I > won't press it. Ack, will drop. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:625: kAllocatorName, true); On 2016/10/14 13:56:56, bcwhite wrote: > On 2016/10/14 04:54:09, lawrencewu wrote: > > On 2016/10/13 18:41:15, bcwhite wrote: > > > On 2016/10/13 14:28:02, bcwhite wrote: > > > > Since you're declaring the allocator as read-only, you should GetAsObject > as > > > > const objects. > > > > > > Actually, if you make this a "const" object then you should get a compiler > > error > > > when trying to get a non-const pointer to objects. > > > > Ack, will ignore this. > > No, seriously. Make it "const". This case is exactly why I put "const" support > in the object to begin with. Oh, my bad, I misunderstood you (thought your second comment meant it couldn't be done because of the compiler). I made the allocator const and made GetAsObject get const FieldTrialEntries. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:748: if (field_trial_allocator == nullptr) { On 2016/10/14 04:54:09, lawrencewu wrote: > On 2016/10/13 14:28:02, bcwhite wrote: > > Are all calls to this method guaranteed to be on the same thread? If not, you > > have a race condition creating the allocator. After that, operations (below) > on > > the allocator are thread-safe. > > I'm not too sure about this. I'll do some investigation, but first, @asvitkine, > do you have any insight on this? Turns out that it is not guaranteed to be thread-safe, so I wrapped the instantiation of the allocator in a lock. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.h:329: static base::SharedPersistentMemoryAllocator* field_trial_allocator; On 2016/10/14 13:56:56, bcwhite wrote: > On 2016/10/14 04:54:10, lawrencewu wrote: > > On 2016/10/13 14:28:02, bcwhite wrote: > > > I don't suppose there is a more generic place to hold this? It could be > > > "child_process_info_allocator" or something, the idea being that it could be > > > then (re-)used if/when other information comes along that would benefit from > > > being passed this way. > > > > While that would be nice, I'm reluctant to do that now since I don't see an > > immediate need for that yet and it would probably require extra effort to > hoist > > this variable up into a more appropriate class. (This CL itself is already > kind > > of an effort to hoist the shared memory from gpu_process_host.cc and > > render_process_host_impl.cc) > > That's fine. Add a comment saying that such _could_ be done if extra data > passing is desired. And keep it in mind as you develop as sometimes there are > choices that would make such a future effort easier. Done. https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:42: const uint32_t kFieldTrialType = 0xABA17E13 + 1; // Version #1 of type On 2016/10/14 14:23:46, bcwhite wrote: > // SHA1(FieldTrialEntry) v1 > > So that others know where this number comes from. Done. https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:58: char* GetTrialName(FieldTrialEntry* fte) { On 2016/10/17 14:46:50, Alexei Svitkine (slow) wrote: > Make this actually a member of FieldTrialEntry - i.e. a struct can have methods > just like a class. > > Then, you don't need |fte| parameter, because you can use |this|. Done. https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:59: char* src = (char*)fte; On 2016/10/17 14:46:50, Alexei Svitkine (slow) wrote: > Per the style guide, you must use C++ style casts rather than C casts. Done. https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:756: FieldTrialList::UpdateFieldTrialAllocator(field_trial); On 2016/10/17 14:46:50, Alexei Svitkine (slow) wrote: > Please make this lazy. > > That is, someone could use base/ and even the FieldTrial API without ever > needing to create a child process or give it experiment state. > > For those use cases, we don't want to create the shared memory eagerly because > it's wasteful. Instead, make the first call to AppendFieldTrialHandleIfNeeded() > trigger the creation of it originally and then thereafter append to it if it's > been created already. Done. https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:772: std::move(shm), 0, kAllocatorName, false); On 2016/10/14 14:23:46, bcwhite wrote: > If you call > > allocator->CreateTrackingHistograms(kAllocatorName); > > You'll get two histograms added to UMA that report the number of allocations and > the amount of memory used. > > See existing UMA.PersistentAllocator.* entries in > tools/metrics/histograms/histograms.xml. > > The catch is that the "UsedPct" entry is updated only with a manual call so at > some point after all the field trials have been updated, you need to call > > allocator->UpdateTrackingHistograms(); > > With this you'll be able to tell if your memory segment is large enough once its > out in the field. Keep the maximum utilization under 75% and you'll be okay. asvitkine and I decided to punt on this for later, so I added a todo comment. This feature won't be going live in this CL since it is hidden behind a boolean anyway. https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:793: field_trial_allocator->Allocate(size, kFieldTrialType); On 2016/10/14 14:23:46, bcwhite wrote: > You need to handle the failure case here. If the field trial list exceeds the > memory size, this will return a null reference which will become nullptr down > below. As it stands, Chrome will crash if there are too many field trials to > store -- an accident waiting to happen. Done.
https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:57: char* GetTrialName() const { const char* Same below. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:618: shm.get()->Map(4 << 10); // 4 KiB = one page Shouldn't this be using field_trial_length? https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:636: base::HandlesToInheritVector& handles) { Parameters shouldn't be passed by non-const refs, per the style guide. Instead, pass by pointer. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:655: FieldTrialList::UpdateFieldTrialAllocator(registered.second); Wouldn't this cause entries to be added multiple times each time the function is called? If not, please add a comment explaining it. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:678: const FieldTrialEntry* fte = Nit: fte -> entry as acronyms are discouraged generally. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:681: char* group_name = fte->GetGroupName(); Just inline these into the CreateFieldTrial() call below. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:789: void FieldTrialList::UpdateFieldTrialAllocator(FieldTrial* field_trial) { This should be in the anonymous namespace at the top of the file if it's not used outside of the .cc file. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:818: memcpy(dst, &fte, sizeof(FieldTrialEntry)); Just cast dst to FTE* and set it directly rather than having a temporary on the stack that you memcpy into.
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...
Address asvitkine's comments. Unsure if we can place a static class method that references private members into the anonymous namespace -- so kept UpdateFieldTrialAllocator as is. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:57: char* GetTrialName() const { On 2016/10/19 16:32:49, Alexei Svitkine (slow) wrote: > const char* > > Same below. Done. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:618: shm.get()->Map(4 << 10); // 4 KiB = one page On 2016/10/19 16:32:48, Alexei Svitkine (slow) wrote: > Shouldn't this be using field_trial_length? I don't think so -- allocated memory isn't guaranteed to be contiguous, so to be safe we should map the same amount of memory we mapped in the browser process. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:636: base::HandlesToInheritVector& handles) { On 2016/10/19 16:32:48, Alexei Svitkine (slow) wrote: > Parameters shouldn't be passed by non-const refs, per the style guide. Instead, > pass by pointer. Done. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:655: FieldTrialList::UpdateFieldTrialAllocator(registered.second); On 2016/10/19 16:32:48, Alexei Svitkine (slow) wrote: > Wouldn't this cause entries to be added multiple times each time the function is > called? If not, please add a comment explaining it. Yes, you're right. I moved this up into where the allocator gets instantiated so it should only get called once. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:678: const FieldTrialEntry* fte = On 2016/10/19 16:32:49, Alexei Svitkine (slow) wrote: > Nit: fte -> entry > > as acronyms are discouraged generally. Done. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:681: char* group_name = fte->GetGroupName(); On 2016/10/19 16:32:48, Alexei Svitkine (slow) wrote: > Just inline these into the CreateFieldTrial() call below. Done. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:789: void FieldTrialList::UpdateFieldTrialAllocator(FieldTrial* field_trial) { On 2016/10/19 16:32:48, Alexei Svitkine (slow) wrote: > This should be in the anonymous namespace at the top of the file if it's not > used outside of the .cc file. This function references the |field_trial_allocator| private member so I don't think we can do that. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:818: memcpy(dst, &fte, sizeof(FieldTrialEntry)); On 2016/10/19 16:32:49, Alexei Svitkine (slow) wrote: > Just cast dst to FTE* and set it directly rather than having a temporary on the > stack that you memcpy into. Done.
https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:618: shm.get()->Map(4 << 10); // 4 KiB = one page On 2016/10/19 20:18:26, lawrencewu wrote: > On 2016/10/19 16:32:48, Alexei Svitkine (slow) wrote: > > Shouldn't this be using field_trial_length? > > I don't think so -- allocated memory isn't guaranteed to be contiguous, so to be > safe we should map the same amount of memory we mapped in the browser process. But that part can be passed from the browser using field_trial_length part of the command line flag? I'm thinking of a case where we may need more than 4KB of space - i.e. the browser makes the decision of how big the segment is and tells child processes this size, rather than child processes assuming they know the size. I guess at this point it's moot since it's hardcoded in both places, so if you want you can add a TODO about it instead. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:655: FieldTrialList::UpdateFieldTrialAllocator(registered.second); On 2016/10/19 20:18:26, lawrencewu wrote: > On 2016/10/19 16:32:48, Alexei Svitkine (slow) wrote: > > Wouldn't this cause entries to be added multiple times each time the function > is > > called? If not, please add a comment explaining it. > > Yes, you're right. I moved this up into where the allocator gets instantiated so > it should only get called once. Can this be covered by a unit test? https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:789: void FieldTrialList::UpdateFieldTrialAllocator(FieldTrial* field_trial) { On 2016/10/19 20:18:25, lawrencewu wrote: > On 2016/10/19 16:32:48, Alexei Svitkine (slow) wrote: > > This should be in the anonymous namespace at the top of the file if it's not > > used outside of the .cc file. > > This function references the |field_trial_allocator| private member so I don't > think we can do that. If field_trial_allocator is not used outside of the .cc file, put it into the anon namespace too and prefix it with g_ for the naming convention.
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...
Address comments. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:618: shm.get()->Map(4 << 10); // 4 KiB = one page On 2016/10/19 20:39:36, Alexei Svitkine (slow) wrote: > On 2016/10/19 20:18:26, lawrencewu wrote: > > On 2016/10/19 16:32:48, Alexei Svitkine (slow) wrote: > > > Shouldn't this be using field_trial_length? > > > > I don't think so -- allocated memory isn't guaranteed to be contiguous, so to > be > > safe we should map the same amount of memory we mapped in the browser process. > > But that part can be passed from the browser using field_trial_length part of > the command line flag? I'm thinking of a case where we may need more than 4KB of > space - i.e. the browser makes the decision of how big the segment is and tells > child processes this size, rather than child processes assuming they know the > size. > > I guess at this point it's moot since it's hardcoded in both places, so if you > want you can add a TODO about it instead. Ah I see, yes that can be passed. Done. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:655: FieldTrialList::UpdateFieldTrialAllocator(registered.second); On 2016/10/19 20:39:36, Alexei Svitkine (slow) wrote: > On 2016/10/19 20:18:26, lawrencewu wrote: > > On 2016/10/19 16:32:48, Alexei Svitkine (slow) wrote: > > > Wouldn't this cause entries to be added multiple times each time the > function > > is > > > called? If not, please add a comment explaining it. > > > > Yes, you're right. I moved this up into where the allocator gets instantiated > so > > it should only get called once. > > Can this be covered by a unit test? Done. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:789: void FieldTrialList::UpdateFieldTrialAllocator(FieldTrial* field_trial) { On 2016/10/19 20:39:36, Alexei Svitkine (slow) wrote: > On 2016/10/19 20:18:25, lawrencewu wrote: > > On 2016/10/19 16:32:48, Alexei Svitkine (slow) wrote: > > > This should be in the anonymous namespace at the top of the file if it's not > > > used outside of the .cc file. > > > > This function references the |field_trial_allocator| private member so I don't > > think we can do that. > > If field_trial_allocator is not used outside of the .cc file, put it into the > anon namespace too and prefix it with g_ for the naming convention. Moved.
https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1152: TEST(FieldTrialListTest, InstantiateAllocator) { This test would pass if the function didn't add any trials either, right? How about having a test that actually ensures that all the trials are added? One way you could do it is to test both the serialization and deserialization. For example, create a field trial list with some trials, serialize it to the allocator, then create a new field trial list and deserialize from the allocator and check that the contents match (e.g. by comparing the field trial states string). https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1156: base::MakeUnique<base::MockEntropyProvider>()); You can just pass in nullptr. https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1160: nullptr); EXPECT_EQ or just EXPECT_FALSE https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1168: base::FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded(); After this test runs, it sounds like it will leave a field trial allocator that was created. That doesn't seem great - because that may affect functioning of other tests. We should clear it so that there's no side effects from running the test. Actually, thinking about it more, how about making the allocator be a member of the FieldTrialList object? This way, it's lifetime is tied to the FieldTrialList object and so any tests that already create/delete the object will correctly reset the allocator too. To make this work with my suggestion of testing the serialization and deserialization, I suggest making a helper function that takes the allocator object as a parameter, so that the serialize/deserialize can use that function for testing. https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1173: EXPECT_TRUE(memory == new_memory); EXPECT_EQ
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:629: shm.get()->Map(field_trial_length); Map() will return false if it fails which can happen even on otherwise healthy systems. You should handle this case or it'll crash later on. https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:695: g_field_trial_allocator->shared_memory()->mapped_size(); Instead of UpdateTrackingHistograms you could also create a histogram here and get the used() size from the allocator directly. Perhaps a better option; doesn't have to be done now.
https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:816: if (g_field_trial_allocator == nullptr) Since we've established that there could be parallel calls to the method that does the original construction of this global object then there is also a thread-safety issue here. You're reading a non-atomic variable without a lock when that variable could be written on other threads. It's a write-once of a machine word so although there are theoretical problems, it's not really a practical one... with THIS variable. However, there is another potential problem: Without atomic release-store and acquire-load operations, there is no ordering of memory accesses. That means that this pointer could be seen with a valid value but the actual object being pointed to has not yet been committed to RAM. Crash. Boom. Bang. Good luck reproducing that under test. The TSAN bot may not complain because I don't think there are any multi-threaded tests to trigger this condition, but that could change in the future. Choices are: 1) Acquire the global lock each time before reading this variable. 2) Use an AtomicWord and reinterpret_cast the pointer to/from it. See histogram_macros.h for an example. You could (but shouldn't) take this several steps further and avoid the global lock completely -- see lazy_instance.h if you're curious how. 3) Create a new class that has an Allocator member, takes no ctor parameters, and initializes that member as desired when constructed. Use a base::LazyInstance<MyType>::Leaky as the type for your global variable and access the Allocator member within. I'd go with #3 if practical, especially if it means you can lose the global lock.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Address comments. https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:629: shm.get()->Map(field_trial_length); On 2016/10/20 21:14:06, bcwhite wrote: > Map() will return false if it fails which can happen even on otherwise healthy > systems. You should handle this case or it'll crash later on. Done. https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1152: TEST(FieldTrialListTest, InstantiateAllocator) { On 2016/10/20 17:50:29, Alexei Svitkine (slow) wrote: > This test would pass if the function didn't add any trials either, right? > > How about having a test that actually ensures that all the trials are added? > > One way you could do it is to test both the serialization and deserialization. > For example, create a field trial list with some trials, serialize it to the > allocator, then create a new field trial list and deserialize from the allocator > and check that the contents match (e.g. by comparing the field trial states > string). Done. https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1156: base::MakeUnique<base::MockEntropyProvider>()); On 2016/10/20 17:50:29, Alexei Svitkine (slow) wrote: > You can just pass in nullptr. Done. https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1160: nullptr); On 2016/10/20 17:50:29, Alexei Svitkine (slow) wrote: > EXPECT_EQ or just EXPECT_FALSE Done. https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1168: base::FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded(); On 2016/10/20 17:50:29, Alexei Svitkine (slow) wrote: > After this test runs, it sounds like it will leave a field trial allocator that > was created. That doesn't seem great - because that may affect functioning of > other tests. We should clear it so that there's no side effects from running the > test. > > Actually, thinking about it more, how about making the allocator be a member of > the FieldTrialList object? This way, it's lifetime is tied to the FieldTrialList > object and so any tests that already create/delete the object will correctly > reset the allocator too. > > To make this work with my suggestion of testing the serialization and > deserialization, I suggest making a helper function that takes the allocator > object as a parameter, so that the serialize/deserialize can use that function > for testing. Done. https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1173: EXPECT_TRUE(memory == new_memory); On 2016/10/20 17:50:29, Alexei Svitkine (slow) wrote: > EXPECT_EQ Done.
https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.cc:626: if (shm.get()->Map(field_trial_length)) I think we should treat this failure as fatal (i.e. causing a sad tab). Otherwise, we have silent errors were field trials don't get created. I see this being done in other places in child processes: https://cs.chromium.org/chromium/src/content/child/child_shared_bitmap_manage... So we could do something similar. So we can make it call base::TerminateBecauseOutOfMemory(alloc_size); if it fails. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.cc:762: if (kUseSharedMemoryForFieldTrials) { Nit: No {}'s for 1-line ifs. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.cc:776: if (global_->field_trial_allocator == nullptr) { Nit: Change this to != and an early return https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.cc:795: HANDLE src = Can you make a helper function for this in the anon namespace? e.g. global_->readonly_allocater_handle = CreateReadOnlyHandle(global_->field_trial_allocator); https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.cc:800: &(global_->readonly_allocator_handle), access, true, 0); Nit: No need for ()'s https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.cc:807: FieldTrial* field_trial) { Sorry for going back and forth on this, but how about making this function be a member function of FieldTrial? Then, you don't need a param since |this| is already a FieldTrial*. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.h:485: static void AppendFieldTrialHandleIfNeeded( Add a comment, please. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.h:541: // Called in NotifyFieldTrialGroupSelection() and afterinstantiating the Nit: I suggest removing the first sentence - the comment should mention what the function does and where it's called from is already obvious from the rest of the code. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.h:587: std::unique_ptr<base::SharedPersistentMemoryAllocator> field_trial_allocator; Add _ at the end https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.h:591: // twice. "stored here since we need it twice" is very vague and doesn't tell me what it's talking about. Be more explicit, e.g.: "Needs to be a member variable because it's needed from both CopyFieldTrialStateToFlags() and AppendFieldTrialHandleIfNeeded()." https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.h:592: HANDLE readonly_allocator_handle; Add _ at the end
Address comments. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.cc:626: if (shm.get()->Map(field_trial_length)) On 2016/10/21 18:20:26, Alexei Svitkine (slow) wrote: > I think we should treat this failure as fatal (i.e. causing a sad tab). > Otherwise, we have silent errors were field trials don't get created. > > I see this being done in other places in child processes: > > https://cs.chromium.org/chromium/src/content/child/child_shared_bitmap_manage... > > So we could do something similar. So we can make it call > base::TerminateBecauseOutOfMemory(alloc_size); if it fails. Done. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.cc:762: if (kUseSharedMemoryForFieldTrials) { On 2016/10/21 18:20:26, Alexei Svitkine (slow) wrote: > Nit: No {}'s for 1-line ifs. Done. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.cc:776: if (global_->field_trial_allocator == nullptr) { On 2016/10/21 18:20:26, Alexei Svitkine (slow) wrote: > Nit: Change this to != and an early return Done. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.cc:795: HANDLE src = On 2016/10/21 18:20:26, Alexei Svitkine (slow) wrote: > Can you make a helper function for this in the anon namespace? > > e.g. global_->readonly_allocater_handle = > CreateReadOnlyHandle(global_->field_trial_allocator); Done. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.cc:800: &(global_->readonly_allocator_handle), access, true, 0); On 2016/10/21 18:20:26, Alexei Svitkine (slow) wrote: > Nit: No need for ()'s Acknowledged but don't need this anymore. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.cc:807: FieldTrial* field_trial) { On 2016/10/21 18:20:26, Alexei Svitkine (slow) wrote: > Sorry for going back and forth on this, but how about making this function be a > member function of FieldTrial? Then, you don't need a param since |this| is > already a FieldTrial*. Done. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.h:485: static void AppendFieldTrialHandleIfNeeded( On 2016/10/21 18:20:26, Alexei Svitkine (slow) wrote: > Add a comment, please. Done. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.h:541: // Called in NotifyFieldTrialGroupSelection() and afterinstantiating the On 2016/10/21 18:20:26, Alexei Svitkine (slow) wrote: > Nit: I suggest removing the first sentence - the comment should mention what the > function does and where it's called from is already obvious from the rest of the > code. Done. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.h:587: std::unique_ptr<base::SharedPersistentMemoryAllocator> field_trial_allocator; On 2016/10/21 18:20:26, Alexei Svitkine (slow) wrote: > Add _ at the end Done. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.h:591: // twice. On 2016/10/21 18:20:26, Alexei Svitkine (slow) wrote: > "stored here since we need it twice" is very vague and doesn't tell me what it's > talking about. > > Be more explicit, e.g.: "Needs to be a member variable because it's needed from > both CopyFieldTrialStateToFlags() and AppendFieldTrialHandleIfNeeded()." Done. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_tri... base/metrics/field_trial.h:592: HANDLE readonly_allocator_handle; On 2016/10/21 18:20:26, Alexei Svitkine (slow) wrote: > Add _ at the end Done.
Thanks, almost there I think - these should the last big set of comments. :) https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:158: HANDLE dst; Nit: Move this right before it's needed i.e. above DuplicateHandle() https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:162: ::DuplicateHandle(process, src, process, &dst, access, true, 0); This function can potentially fail - as indicated by the return value. Let's fall back to the old command line path in that case for now. (We can revisit later.) https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:776: if (kUseSharedMemoryForFieldTrials) Nit: {} since now it's more than one line. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:826: FieldTrial::State trial_state; Nit: Do you ned FieldTrial:: here? https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:827: this->GetState(&trial_state); Nit: No need for this-> https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:829: std::string trial_name; I don't think these string copies here are necessary. You should be able to copy directly from the string piece to the entry. Now, the StringPiece is not guaranteed to have the null terminator, but that's fine because you can just reserve the extra byte (which you already account for) and set it to 0 explicitly (unless the memory is already guaranteed to be 0-initialized in which case it's not necessary). https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:849: *dst = entry; Nit: Instead of the extra copy, how about: FieldTrialEntry* entry = allocator->GetAsObject<FieldTrialEntry>(...); entry->activated = trial_state.activated; entry->group_name_offset = group_name_offset; This way, you don't create an extra struct on the stack and copy into out - and also there's less variables in this function which makes the code simpler. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.h:592: HANDLE readonly_allocator_handle_; Initialize this please.
One more thing: Please format your CL description such that you have a 1-line summary of the CL as the first line, then an empty line and then the more detailed description.
Description was changed from ========== Use SharedPersistentMemoryAllocator to share field trial state between processes instead of passing it via a string in shared memory. Adds the allocator to the base::FieldTrialList singleton. BUG=653874 ========== to ========== Use SharedPersistentMemoryAllocator to share field trial state Change the method by which we share field trial state from using a SharedMemory class to SharedPersistentMemoryAllocator. Adds this allocator to the base::FieldTrialList singleton, so there is only one copy of this state on the browser process vs. a copy for each process host which is how it currently works (from https://codereview.chromium.org/2365273004/) BUG=653874 ==========
Description was changed from ========== Use SharedPersistentMemoryAllocator to share field trial state Change the method by which we share field trial state from using a SharedMemory class to SharedPersistentMemoryAllocator. Adds this allocator to the base::FieldTrialList singleton, so there is only one copy of this state on the browser process vs. a copy for each process host which is how it currently works (from https://codereview.chromium.org/2365273004/) BUG=653874 ========== to ========== Use SharedPersistentMemoryAllocator to share field trial state Change the method by which we share field trial state from using a SharedMemory class to SharedPersistentMemoryAllocator. Adds this allocator to the base::FieldTrialList singleton, so there is only one copy of this state on the browser process vs. a copy for each process host which is how it currently works (from https://codereview.chromium.org/2365273004/) BUG=653874 ==========
Addressed comments. https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:695: g_field_trial_allocator->shared_memory()->mapped_size(); On 2016/10/20 21:14:06, bcwhite wrote: > Instead of UpdateTrackingHistograms you could also create a histogram here and > get the used() size from the allocator directly. Perhaps a better option; > doesn't have to be done now. Ack, will do later. https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:816: if (g_field_trial_allocator == nullptr) On 2016/10/21 13:48:29, bcwhite wrote: > Since we've established that there could be parallel calls to the method that > does the original construction of this global object then there is also a > thread-safety issue here. You're reading a non-atomic variable without a lock > when that variable could be written on other threads. > > It's a write-once of a machine word so although there are theoretical problems, > it's not really a practical one... with THIS variable. However, there is > another potential problem: Without atomic release-store and acquire-load > operations, there is no ordering of memory accesses. That means that this > pointer could be seen with a valid value but the actual object being pointed to > has not yet been committed to RAM. Crash. Boom. Bang. Good luck reproducing > that under test. > > The TSAN bot may not complain because I don't think there are any multi-threaded > tests to trigger this condition, but that could change in the future. > > Choices are: > > 1) Acquire the global lock each time before reading this variable. > > 2) Use an AtomicWord and reinterpret_cast the pointer to/from it. See > histogram_macros.h for an example. You could (but shouldn't) take this several > steps further and avoid the global lock completely -- see lazy_instance.h if > you're curious how. > > 3) Create a new class that has an Allocator member, takes no ctor parameters, > and initializes that member as desired when constructed. Use a > base::LazyInstance<MyType>::Leaky as the type for your global variable and > access the Allocator member within. > > I'd go with #3 if practical, especially if it means you can lose the global > lock. Done. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:158: HANDLE dst; On 2016/10/21 20:52:21, Alexei Svitkine (slow) wrote: > Nit: Move this right before it's needed i.e. above DuplicateHandle() Done. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:162: ::DuplicateHandle(process, src, process, &dst, access, true, 0); On 2016/10/21 20:52:21, Alexei Svitkine (slow) wrote: > This function can potentially fail - as indicated by the return value. > > Let's fall back to the old command line path in that case for now. > > (We can revisit later.) Done. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:776: if (kUseSharedMemoryForFieldTrials) On 2016/10/21 20:52:21, Alexei Svitkine (slow) wrote: > Nit: {} since now it's more than one line. Done. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:826: FieldTrial::State trial_state; On 2016/10/21 20:52:21, Alexei Svitkine (slow) wrote: > Nit: Do you ned FieldTrial:: here? Nope, removed. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:827: this->GetState(&trial_state); On 2016/10/21 20:52:21, Alexei Svitkine (slow) wrote: > Nit: No need for this-> Done. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:829: std::string trial_name; On 2016/10/21 20:52:21, Alexei Svitkine (slow) wrote: > I don't think these string copies here are necessary. > > You should be able to copy directly from the string piece to the entry. > > Now, the StringPiece is not guaranteed to have the null terminator, but that's > fine because you can just reserve the extra byte (which you already account for) > and set it to 0 explicitly (unless the memory is already guaranteed to be > 0-initialized in which case it's not necessary). Done. The memory is guaranteed to be 0-initialized, at least on Windows: "The initial contents of the pages in a file mapping object backed by the operating system paging file are 0 (zero)." from https://msdn.microsoft.com/en-us/library/windows/desktop/aa366537(v=vs.85).aspx as well as: https://cs.chromium.org/chromium/src/base/metrics/persistent_memory_allocator... I believe that on Linux, since we use mkstemp to create the temporary file used as backing for the shared memory, that should be zeroed out as well. On Mac, which uses mach_vm_map(), the memory is also zero-filled: https://books.google.ca/books?id=K8vUkpOXhN4C&pg=PA883&lpg=PA883&dq=mach_vm_m... I am not completely sure about Linux and Mac, though -- maybe erikchen@ knows something we don't. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:849: *dst = entry; On 2016/10/21 20:52:21, Alexei Svitkine (slow) wrote: > Nit: > > Instead of the extra copy, how about: > > FieldTrialEntry* entry = allocator->GetAsObject<FieldTrialEntry>(...); > entry->activated = trial_state.activated; > entry->group_name_offset = group_name_offset; > > This way, you don't create an extra struct on the stack and copy into out - and > also there's less variables in this function which makes the code simpler. Yup that's definitely cleaner. Done. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.h:592: HANDLE readonly_allocator_handle_; On 2016/10/21 20:52:21, Alexei Svitkine (slow) wrote: > Initialize this please. Done. Also initialized the allocator to nullptr too.
https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:658: FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded(); Wait, what happened to this line? Are you removing it because CopyFieldTrialStateToFlag() gets called first? If so, you're putting a requirement on the caller to do that which might break in the future. I suggest either making the APIs resilient to either order - or to document the expected order thoroughly - i.e. both in API comments in the header and also above the call to this function from the place it's called. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:776: if (kUseSharedMemoryForFieldTrials) On 2016/10/23 20:01:56, lawrencewu wrote: > On 2016/10/21 20:52:21, Alexei Svitkine (slow) wrote: > > Nit: {} since now it's more than one line. > > Done. Doesn't look done to me. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:829: std::string trial_name; On 2016/10/23 20:01:56, lawrencewu wrote: > On 2016/10/21 20:52:21, Alexei Svitkine (slow) wrote: > > I don't think these string copies here are necessary. > > > > You should be able to copy directly from the string piece to the entry. > > > > Now, the StringPiece is not guaranteed to have the null terminator, but that's > > fine because you can just reserve the extra byte (which you already account > for) > > and set it to 0 explicitly (unless the memory is already guaranteed to be > > 0-initialized in which case it's not necessary). > > Done. The memory is guaranteed to be 0-initialized, at least on Windows: "The > initial contents of the pages in a file mapping object backed by the operating > system paging file are 0 (zero)." from > https://msdn.microsoft.com/en-us/library/windows/desktop/aa366537(v=vs.85).aspx > > as well as: > https://cs.chromium.org/chromium/src/base/metrics/persistent_memory_allocator... > > I believe that on Linux, since we use mkstemp to create the temporary file used > as backing for the shared memory, that should be zeroed out as well. On Mac, > which uses mach_vm_map(), the memory is also zero-filled: > https://books.google.ca/books?id=K8vUkpOXhN4C&pg=PA883&lpg=PA883&dq=mach_vm_m... > > I am not completely sure about Linux and Mac, though -- maybe erikchen@ knows > something we don't. Let's just explicitly 0 the null terminator characters, if you're not entirely sure. It's super cheap to do and this way guarantees there's no issues. https://codereview.chromium.org/2412113002/diff/300001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:55: bool activated; I don't think your current code updates this when the trial gets activated - which is the whole point of keeping it here. To do that, you probably need to keep a pointer to FTE from each FieldTrial object - or else have to look it up each time. I'm OK with you putting a TODO about it here and addressing it in a follow-up CL - given that we're not turning this on yet. https://codereview.chromium.org/2412113002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:172: return NULL; Don't use NULL in new code. Use nullptr - unless there's a different convention for HANDLE values. Also, be consistent with what you have in the header file for readonly_allocator_handle_ - where you currently use 0. https://codereview.chromium.org/2412113002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:663: handles->push_back(global_->readonly_allocator_handle_); You only want to do this if it's no null, right? https://codereview.chromium.org/2412113002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:831: GetState(&trial_state); This function has a return value - if it's false, |trial_state| doesn't get filled in. You want to do an early return if it's false, probably with a comment.
Address comments. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:658: FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded(); On 2016/10/24 14:40:27, Alexei Svitkine (slow) wrote: > Wait, what happened to this line? > > Are you removing it because CopyFieldTrialStateToFlag() gets called first? If > so, you're putting a requirement on the caller to do that which might break in > the future. > > I suggest either making the APIs resilient to either order - or to document the > expected order thoroughly - i.e. both in API comments in the header and also > above the call to this function from the place it's called. Added the line back. For some reason I thought I wouldn't be able to implement the fallback behavior without removing this line, but now I realize that the handle would still be null, since this function is idempotent. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:776: if (kUseSharedMemoryForFieldTrials) On 2016/10/24 14:40:27, Alexei Svitkine (slow) wrote: > On 2016/10/23 20:01:56, lawrencewu wrote: > > On 2016/10/21 20:52:21, Alexei Svitkine (slow) wrote: > > > Nit: {} since now it's more than one line. > > > > Done. > > Doesn't look done to me. Woops, actually done now. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_tri... base/metrics/field_trial.cc:829: std::string trial_name; On 2016/10/24 14:40:27, Alexei Svitkine (slow) wrote: > On 2016/10/23 20:01:56, lawrencewu wrote: > > On 2016/10/21 20:52:21, Alexei Svitkine (slow) wrote: > > > I don't think these string copies here are necessary. > > > > > > You should be able to copy directly from the string piece to the entry. > > > > > > Now, the StringPiece is not guaranteed to have the null terminator, but > that's > > > fine because you can just reserve the extra byte (which you already account > > for) > > > and set it to 0 explicitly (unless the memory is already guaranteed to be > > > 0-initialized in which case it's not necessary). > > > > Done. The memory is guaranteed to be 0-initialized, at least on Windows: "The > > initial contents of the pages in a file mapping object backed by the operating > > system paging file are 0 (zero)." from > > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa366537(v=vs.85).aspx > > > > as well as: > > > https://cs.chromium.org/chromium/src/base/metrics/persistent_memory_allocator... > > > > I believe that on Linux, since we use mkstemp to create the temporary file > used > > as backing for the shared memory, that should be zeroed out as well. On Mac, > > which uses mach_vm_map(), the memory is also zero-filled: > > > https://books.google.ca/books?id=K8vUkpOXhN4C&pg=PA883&lpg=PA883&dq=mach_vm_m... > > > > I am not completely sure about Linux and Mac, though -- maybe erikchen@ knows > > something we don't. > > Let's just explicitly 0 the null terminator characters, if you're not entirely > sure. It's super cheap to do and this way guarantees there's no issues. Sure, done. https://codereview.chromium.org/2412113002/diff/300001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:55: bool activated; On 2016/10/24 14:40:27, Alexei Svitkine (slow) wrote: > I don't think your current code updates this when the trial gets activated - > which is the whole point of keeping it here. > > To do that, you probably need to keep a pointer to FTE from each FieldTrial > object - or else have to look it up each time. > > I'm OK with you putting a TODO about it here and addressing it in a follow-up CL > - given that we're not turning this on yet. I added a comment and will tackle this in the next CL. https://codereview.chromium.org/2412113002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:172: return NULL; On 2016/10/24 14:40:27, Alexei Svitkine (slow) wrote: > Don't use NULL in new code. > > Use nullptr - unless there's a different convention for HANDLE values. Also, be > consistent with what you have in the header file for readonly_allocator_handle_ > - where you currently use 0. Used nullptr and initialized it in the header file to nullptr too. https://codereview.chromium.org/2412113002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:663: handles->push_back(global_->readonly_allocator_handle_); On 2016/10/24 14:40:27, Alexei Svitkine (slow) wrote: > You only want to do this if it's no null, right? Yep, done. https://codereview.chromium.org/2412113002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:831: GetState(&trial_state); On 2016/10/24 14:40:27, Alexei Svitkine (slow) wrote: > This function has a return value - if it's false, |trial_state| doesn't get > filled in. > > You want to do an early return if it's false, probably with a comment. Done.
lgtm % remaining comments Thanks! https://codereview.chromium.org/2412113002/diff/340001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/340001/base/metrics/field_tri... base/metrics/field_trial.cc:834: // get the field trial state, quit early. Nit: Seems like the first part of the comment (the space calculation and no memory) belong further down. So I suggest moving it there. The second part of the comment doesn't say anything beyond what the code does, so you can omit it. (Or you can expand to provide more reason as to *why* it's OK.) https://codereview.chromium.org/2412113002/diff/340001/base/metrics/field_tri... base/metrics/field_trial.cc:862: // Set the null terminator, just in case. Nit: How about "Null terminate the strings." It's not really "just in case" since you said you haven't checked whether there's a guarantee - and if there is, it should be documented by the APIs in Chrome we use with test coverage. https://codereview.chromium.org/2412113002/diff/340001/base/metrics/field_tri... base/metrics/field_trial.cc:865: *end_of_trial_name = '\0'; Nit: How about: trial_name[trial_state.trial_name.size()] = '\0'; And same for the other one.
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: + jam@chromium.org
Address nits. @jam: Would you mind looking at the files under components and content? These basically undo the things done in https://codereview.chromium.org/2365273004/. https://codereview.chromium.org/2412113002/diff/340001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/340001/base/metrics/field_tri... base/metrics/field_trial.cc:834: // get the field trial state, quit early. On 2016/10/24 15:41:56, Alexei Svitkine (slow) wrote: > Nit: Seems like the first part of the comment (the space calculation and no > memory) belong further down. So I suggest moving it there. > > The second part of the comment doesn't say anything beyond what the code does, > so you can omit it. (Or you can expand to provide more reason as to *why* it's > OK.) Moved the comment down and omitted the second part. https://codereview.chromium.org/2412113002/diff/340001/base/metrics/field_tri... base/metrics/field_trial.cc:862: // Set the null terminator, just in case. On 2016/10/24 15:41:57, Alexei Svitkine (slow) wrote: > Nit: How about "Null terminate the strings." > > It's not really "just in case" since you said you haven't checked whether > there's a guarantee - and if there is, it should be documented by the APIs in > Chrome we use with test coverage. Done. https://codereview.chromium.org/2412113002/diff/340001/base/metrics/field_tri... base/metrics/field_trial.cc:865: *end_of_trial_name = '\0'; On 2016/10/24 15:41:57, Alexei Svitkine (slow) wrote: > Nit: How about: > > trial_name[trial_state.trial_name.size()] = '\0'; > > And same for the other one. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Still lgtm, couple final nits I saw. https://codereview.chromium.org/2412113002/diff/420001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/420001/base/metrics/field_tri... base/metrics/field_trial.cc:663: base::HandlesToInheritVector* handles) { Nit: Remove all the base:: prefixes throughout since this code is in base. https://codereview.chromium.org/2412113002/diff/420001/base/metrics/field_tri... base/metrics/field_trial.cc:667: FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded(); Nit: Remove FieldTrialList:: https://codereview.chromium.org/2412113002/diff/420001/base/metrics/field_tri... base/metrics/field_trial.cc:706: FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded(); Nit: Remove FieldTrialList::
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...
address comments. https://codereview.chromium.org/2412113002/diff/420001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/420001/base/metrics/field_tri... base/metrics/field_trial.cc:663: base::HandlesToInheritVector* handles) { On 2016/10/24 19:57:01, Alexei Svitkine (slow) wrote: > Nit: Remove all the base:: prefixes throughout since this code is in base. Done. https://codereview.chromium.org/2412113002/diff/420001/base/metrics/field_tri... base/metrics/field_trial.cc:667: FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded(); On 2016/10/24 19:57:01, Alexei Svitkine (slow) wrote: > Nit: Remove FieldTrialList:: Done. https://codereview.chromium.org/2412113002/diff/420001/base/metrics/field_tri... base/metrics/field_trial.cc:706: FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded(); On 2016/10/24 19:57:01, Alexei Svitkine (slow) wrote: > Nit: Remove FieldTrialList:: Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lawrencewu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2412113002/#ps410018 (title: "gclient sync")
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.
Description was changed from ========== Use SharedPersistentMemoryAllocator to share field trial state Change the method by which we share field trial state from using a SharedMemory class to SharedPersistentMemoryAllocator. Adds this allocator to the base::FieldTrialList singleton, so there is only one copy of this state on the browser process vs. a copy for each process host which is how it currently works (from https://codereview.chromium.org/2365273004/) BUG=653874 ========== to ========== Use SharedPersistentMemoryAllocator to share field trial state Change the method by which we share field trial state from using a SharedMemory class to SharedPersistentMemoryAllocator. Adds this allocator to the base::FieldTrialList singleton, so there is only one copy of this state on the browser process vs. a copy for each process host which is how it currently works (from https://codereview.chromium.org/2365273004/) BUG=653874 ==========
Message was sent while issue was closed.
Committed patchset #24 (id:410018)
Message was sent while issue was closed.
Description was changed from ========== Use SharedPersistentMemoryAllocator to share field trial state Change the method by which we share field trial state from using a SharedMemory class to SharedPersistentMemoryAllocator. Adds this allocator to the base::FieldTrialList singleton, so there is only one copy of this state on the browser process vs. a copy for each process host which is how it currently works (from https://codereview.chromium.org/2365273004/) BUG=653874 ========== to ========== Use SharedPersistentMemoryAllocator to share field trial state Change the method by which we share field trial state from using a SharedMemory class to SharedPersistentMemoryAllocator. Adds this allocator to the base::FieldTrialList singleton, so there is only one copy of this state on the browser process vs. a copy for each process host which is how it currently works (from https://codereview.chromium.org/2365273004/) BUG=653874 Committed: https://crrev.com/0b496492defaf5f2f70e08318c36568ccce9ce87 Cr-Commit-Position: refs/heads/master@{#427378} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/0b496492defaf5f2f70e08318c36568ccce9ce87 Cr-Commit-Position: refs/heads/master@{#427378} |
