Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2560723004:
Add field trial dump and retrieval methods from shared memory (Closed)
|
Created:
4 years ago by lawrencewu Modified:
4 years ago CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd field trial dump and retrieval methods from shared memory
The Breadcrumbs project dumps system state to a persistent file so that it can
be analyzed after a crash. We want to include in that the complete set of field
trials. That means saving them to the file when the browser starts and being
able to retrieve them from the file on a future run; they will be loaded into a
protobuf and uploaded.
BUG=663414
Committed: https://crrev.com/1b688cc0790c38b70a391a6c2bbbb36c7b7628a8
Cr-Commit-Position: refs/heads/master@{#437732}
Patch Set 1 #Patch Set 2 : fix header #Patch Set 3 : fix header #Patch Set 4 : git cl format and rebase #Patch Set 5 : remove duplicate variable #Patch Set 6 : fix getting state #Patch Set 7 : write unittest #Patch Set 8 : add comments #Patch Set 9 : Move to FieldTrialEntry. #
Total comments: 8
Patch Set 10 : bcwhite@ comments #
Total comments: 16
Patch Set 11 : Address asvitkine@ nits. #Patch Set 12 : fix compile issues #Patch Set 13 : Address manzagop nits. #Patch Set 14 : add BASE_EXPORT #
Messages
Total messages: 59 (39 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...
Description was changed from ========== googl# Enter a description of the change. Make AddToAllocator public and take in the allocator. BUG=663414 ========== to ========== Make AddToAllocator public and take in the allocator. BUG=663414 ==========
lawrencewu@chromium.org changed reviewers: + asvitkine@chromium.org, bcwhite@chromium.org
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: 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: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) 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...
bcwhite@: Would you mind commenting in detail the motivation for this? I only understand it vaguely (to be able to see the state of the shared memory during a crash etc.) And of course let me know if this isn't what you wanted.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/08 20:22:41, lawrencewu wrote: > bcwhite@: Would you mind commenting in detail the motivation for this? I only > understand it vaguely (to be able to see the state of the shared memory during a > crash etc.) > > And of course let me know if this isn't what you wanted. The Breadcrumbs project dumps system state to a persistent file so that it can be analyzed after a crash. We want to include in that the complete set of field trials. That means saving them to the file when the browser starts and being able to retrieve them from the file on a future run; they will be loaded into a protobuf and uploaded. So really we're looking for two methods: static void DumpAllFieldTrialsToPersistentAllocator(...); static std::vector<> GetAllFieldTrialsFromPersistentAllocator(...);
bcwhite@chromium.org changed reviewers: + manzagop@chromium.org
Description was changed from ========== Make AddToAllocator public and take in the allocator. BUG=663414 ========== to ========== Make AddToAllocator public and take in the allocator. The Breadcrumbs project dumps system state to a persistent file so that it can be analyzed after a crash. We want to include in that the complete set of field trials. That means saving them to the file when the browser starts and being able to retrieve them from the file on a future run; they will be loaded into a protobuf and uploaded. BUG=663414 ==========
Description was changed from ========== Make AddToAllocator public and take in the allocator. The Breadcrumbs project dumps system state to a persistent file so that it can be analyzed after a crash. We want to include in that the complete set of field trials. That means saving them to the file when the browser starts and being able to retrieve them from the file on a future run; they will be loaded into a protobuf and uploaded. BUG=663414 ========== to ========== Add field trial dump and retrieval methods from shared memory The Breadcrumbs project dumps system state to a persistent file so that it can be analyzed after a crash. We want to include in that the complete set of field trials. That means saving them to the file when the browser starts and being able to retrieve them from the file on a future run; they will be loaded into a protobuf and uploaded. BUG=663414 ==========
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...
Create the two methods Brian requested.
https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... base/metrics/field_trial.cc:1129: FieldTrial::FieldTrialEntry* entry = const https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... base/metrics/field_trial.h:153: PickleIterator GetPickleIterator() const; Can this method be private? https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... base/metrics/field_trial.h:157: bool ReadStringPair(PickleIterator* iter, Can this method be private? https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... base/metrics/field_trial.h:623: // it can be analyzed after a crash. Comment that the pointers are into the persistent memory segment and so are only valid as long as the allocator is valid. https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... base/metrics/field_trial.h:626: PersistentMemoryAllocator* allocator); const PMA*
Address comments. https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... base/metrics/field_trial.cc:1129: FieldTrial::FieldTrialEntry* entry = On 2016/12/09 19:40:28, bcwhite wrote: > const Done. https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... base/metrics/field_trial.h:153: PickleIterator GetPickleIterator() const; On 2016/12/09 19:40:28, bcwhite wrote: > Can this method be private? Done. https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... base/metrics/field_trial.h:157: bool ReadStringPair(PickleIterator* iter, On 2016/12/09 19:40:28, bcwhite wrote: > Can this method be private? Done.
On 2016/12/09 19:47:59, lawrencewu wrote: > Address comments. > > https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... > File base/metrics/field_trial.cc (right): > > https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... > base/metrics/field_trial.cc:1129: FieldTrial::FieldTrialEntry* entry = > On 2016/12/09 19:40:28, bcwhite wrote: > > const > > Done. > > https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... > File base/metrics/field_trial.h (right): > > https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... > base/metrics/field_trial.h:153: PickleIterator GetPickleIterator() const; > On 2016/12/09 19:40:28, bcwhite wrote: > > Can this method be private? > > Done. > > https://codereview.chromium.org/2560723004/diff/160001/base/metrics/field_tri... > base/metrics/field_trial.h:157: bool ReadStringPair(PickleIterator* iter, > On 2016/12/09 19:40:28, bcwhite wrote: > > Can this method be private? > > Done. The other two comments are done too, I just forgot to mark them.
lgtm
lgtm % comments https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.cc:1124: std::vector<const FieldTrial::FieldTrialEntry*> field_trial_entries; Nit: How about just |entries| to make the name a bit more concise. https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.h:172: }; Nit: Add DISALLOW_COPY_AND_ASSIGN(FieldTrialEntry)? https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.h:618: // We want to be able to dump field trial state to an allocator so that it Nit: Can you rephrase this comment and the one below without using "we want" language. i.e. describe what it does. https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.h:629: const PersistentMemoryAllocator* allocator); Can this be passed by const& instead of const*?
https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.h:172: }; On 2016/12/09 20:08:43, Alexei Svitkine (slow) wrote: > Nit: Add DISALLOW_COPY_AND_ASSIGN(FieldTrialEntry)? Allocator objects are always POD so can always be copied and assigned. You may get an error if you add ctor definitions because the compiler may no longer consider it a POD.
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...
On 2016/12/09 20:13:30, bcwhite wrote: > https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... > File base/metrics/field_trial.h (right): > > https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... > base/metrics/field_trial.h:172: }; > On 2016/12/09 20:08:43, Alexei Svitkine (slow) wrote: > > Nit: Add DISALLOW_COPY_AND_ASSIGN(FieldTrialEntry)? > > Allocator objects are always POD so can always be copied and assigned. You may > get an error if you add ctor definitions because the compiler may no longer > consider it a POD. Yeah, looks like the compiler is complaining about that.
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@ comments https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.cc:1124: std::vector<const FieldTrial::FieldTrialEntry*> field_trial_entries; On 2016/12/09 20:08:43, Alexei Svitkine (slow) wrote: > Nit: How about just |entries| to make the name a bit more concise. Done. https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.h:618: // We want to be able to dump field trial state to an allocator so that it On 2016/12/09 20:08:43, Alexei Svitkine (slow) wrote: > Nit: Can you rephrase this comment and the one below without using "we want" > language. i.e. describe what it does. Done (as well as below). https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.h:629: const PersistentMemoryAllocator* allocator); On 2016/12/09 20:08:43, Alexei Svitkine (slow) wrote: > Can this be passed by const& instead of const*? Done.
Cool! My main concern is that IIUC this is a one time dump, meaning if a field trial is activated after the dump, then we won't know about it. Is this correct? https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.cc:254: StringPiece* group_name) const { nit: DCHECK pointers? That said, other functions in this file don't. So only if the owners agree. https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.cc:263: if (!ReadStringPair(&iter, &tmp, &tmp)) nit: comment to state we skip trial and group name https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.h:140: // structure requires a bump in kFieldTrialType id defined above. nit: update comment as kFieldTrialType is defined in .cc, not above. https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.h:172: }; On 2016/12/09 20:13:29, bcwhite wrote: > On 2016/12/09 20:08:43, Alexei Svitkine (slow) wrote: > > Nit: Add DISALLOW_COPY_AND_ASSIGN(FieldTrialEntry)? > > Allocator objects are always POD so can always be copied and assigned. You may > get an error if you add ctor definitions because the compiler may no longer > consider it a POD. Drive by: if relying on POD / trivial copy, use a is_pod / is_trivial or some such static assert?
https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.h:172: }; On 2016/12/09 21:22:24, manzagop wrote: > On 2016/12/09 20:13:29, bcwhite wrote: > > On 2016/12/09 20:08:43, Alexei Svitkine (slow) wrote: > > > Nit: Add DISALLOW_COPY_AND_ASSIGN(FieldTrialEntry)? > > > > Allocator objects are always POD so can always be copied and assigned. You > may > > get an error if you add ctor definitions because the compiler may no longer > > consider it a POD. > > Drive by: if relying on POD / trivial copy, use a is_pod / is_trivial or some > such static assert? Already exists. See PersistentMemoryAllocator::GetAsObject<>()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Address manzagop@ comments. https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.cc:254: StringPiece* group_name) const { On 2016/12/09 21:22:24, manzagop wrote: > nit: DCHECK pointers? That said, other functions in this file don't. So only if > the owners agree. I'd like to punt on this -- it can go in another CL. https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.cc:263: if (!ReadStringPair(&iter, &tmp, &tmp)) On 2016/12/09 21:22:24, manzagop wrote: > nit: comment to state we skip trial and group name Done. https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2560723004/diff/180001/base/metrics/field_tri... base/metrics/field_trial.h:140: // structure requires a bump in kFieldTrialType id defined above. On 2016/12/09 21:22:24, manzagop wrote: > nit: update comment as kFieldTrialType is defined in .cc, not above. Done.
The CQ bit was checked by lawrencewu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2560723004/#ps240001 (title: "Address manzagop nits.")
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 lawrencewu@chromium.org
The CQ bit was checked by lawrencewu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2560723004/#ps260001 (title: "add BASE_EXPORT")
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
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by lawrencewu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1481335726561190,
"parent_rev": "8b043b5fa4b9b8f5e8ea410f724844957a71d280", "commit_rev":
"e122022a44ce425877e75f99e55295815485cf69"}
Message was sent while issue was closed.
Description was changed from ========== Add field trial dump and retrieval methods from shared memory The Breadcrumbs project dumps system state to a persistent file so that it can be analyzed after a crash. We want to include in that the complete set of field trials. That means saving them to the file when the browser starts and being able to retrieve them from the file on a future run; they will be loaded into a protobuf and uploaded. BUG=663414 ========== to ========== Add field trial dump and retrieval methods from shared memory The Breadcrumbs project dumps system state to a persistent file so that it can be analyzed after a crash. We want to include in that the complete set of field trials. That means saving them to the file when the browser starts and being able to retrieve them from the file on a future run; they will be loaded into a protobuf and uploaded. BUG=663414 Review-Url: https://codereview.chromium.org/2560723004 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Add field trial dump and retrieval methods from shared memory The Breadcrumbs project dumps system state to a persistent file so that it can be analyzed after a crash. We want to include in that the complete set of field trials. That means saving them to the file when the browser starts and being able to retrieve them from the file on a future run; they will be loaded into a protobuf and uploaded. BUG=663414 Review-Url: https://codereview.chromium.org/2560723004 ========== to ========== Add field trial dump and retrieval methods from shared memory The Breadcrumbs project dumps system state to a persistent file so that it can be analyzed after a crash. We want to include in that the complete set of field trials. That means saving them to the file when the browser starts and being able to retrieve them from the file on a future run; they will be loaded into a protobuf and uploaded. BUG=663414 Committed: https://crrev.com/1b688cc0790c38b70a391a6c2bbbb36c7b7628a8 Cr-Commit-Position: refs/heads/master@{#437732} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/1b688cc0790c38b70a391a6c2bbbb36c7b7628a8 Cr-Commit-Position: refs/heads/master@{#437732} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
