Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2754483002:
Add analyzer support for multiple processes. (Closed)
|
Created:
3 years, 9 months ago by bcwhite Modified:
3 years, 8 months ago Reviewers:
manzagop (departed) CC:
chromium-reviews, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd analyzer support for multiple processes.
BUG=620813
Review-Url: https://codereview.chromium.org/2754483002
Cr-Commit-Position: refs/heads/master@{#461141}
Committed: https://chromium.googlesource.com/chromium/src/+/e7aca0670b59b9062005a9b5a96cfb2d29743682
Patch Set 1 #Patch Set 2 : fixed build problems #Patch Set 3 : rebased #
Total comments: 24
Patch Set 4 : rebased #Patch Set 5 : addressed review comments by manzagop #
Total comments: 4
Patch Set 6 : fixed test #Patch Set 7 : addressed review comments by manzagop #
Messages
Total messages: 58 (49 generated)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
bcwhite@chromium.org changed reviewers: + manzagop@chromium.org
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 bcwhite@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ping
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_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 bcwhite@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: This issue passed the CQ dry run.
Sorry, last week was hectic. Looking now.
Nice! A few comments/questions. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.cc:123: create_stamp <= analysis_stamp_); Not sure I follow this condition. Should it be "while 1) not the right process OR 2) the process was created after analysis began" (ie || create_stamp >= analysis_stamp)? Might be worth adding a test with multiple processes. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.cc:274: // record has been marked "free" since the type will not match. Update comment: retrieval of kTypeIdAny can't fail? https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.cc:276: memory_ref, 0, PersistentMemoryAllocator::kSizeAny); Is 0 the same as kTypeIdAny? https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.cc:307: case GlobalActivityTracker::kTypeIdProcessDataRecord: { Looks like a process that had no threads won't be added to process_ids_. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.cc:332: // Reverse the list of PIDs so that they get popped in the order found. Is the order important? We don't seem to care about it for threads. Could we just drop the pid vector? https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.h:147: // Iterates over all known valid processes and returns their PIDs or zero if So... can zero be a valid pid? (just checking) https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.h:159: // analyzer pointers are invalidated when GetFirstProcess() is called. May be worth mentioning GetNextAnalyzer calls for different pids can't be interleaved. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.h:224: // A set of all known process IDs. Looks like this empties itself as we iterate over processes. Likely worth expanding on comment. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:438: // Find any new data that may have been added by an active instance of this Can you say more about what/where this other instance is? IIUC there'd need to be some locking for the writes not to step on each other's toes below? https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:584: memory_ = nullptr; This behavior should be mentioned in the .h.
The CQ bit was checked by bcwhite@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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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 bcwhite@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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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_...)
The CQ bit was checked by bcwhite@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...
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.cc:123: create_stamp <= analysis_stamp_); On 2017/03/21 21:10:05, manzagop wrote: > Not sure I follow this condition. Should it be "while 1) not the right process > OR 2) the process was created after analysis began" (ie || create_stamp >= > analysis_stamp)? Done. > Might be worth adding a test with multiple processes. That's pretty complicated and shouldn't be necessary since it doesn't really behave any differently than standard multi-threading except for the PID value that gets stored. Instead, I'll let the PID be override-able during construction which should allow it to be tested even in a single thread. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.cc:274: // record has been marked "free" since the type will not match. On 2017/03/21 21:10:05, manzagop wrote: > Update comment: retrieval of kTypeIdAny can't fail? Done. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.cc:276: memory_ref, 0, PersistentMemoryAllocator::kSizeAny); On 2017/03/21 21:10:05, manzagop wrote: > Is 0 the same as kTypeIdAny? Done. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.cc:307: case GlobalActivityTracker::kTypeIdProcessDataRecord: { On 2017/03/21 21:10:05, manzagop wrote: > Looks like a process that had no threads won't be added to process_ids_. Done. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.cc:332: // Reverse the list of PIDs so that they get popped in the order found. On 2017/03/21 21:10:05, manzagop wrote: > Is the order important? We don't seem to care about it for threads. Could we > just drop the pid vector? There has to be some list of PIDs though it could be the (merged?) keys of any map. But I think it's worth keeping the order because then the first process (i.e. the Browser) will be the first one returned and that might be useful during analysis. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.h:147: // Iterates over all known valid processes and returns their PIDs or zero if On 2017/03/21 21:10:05, manzagop wrote: > So... can zero be a valid pid? (just checking) No. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.h:159: // analyzer pointers are invalidated when GetFirstProcess() is called. On 2017/03/21 21:10:05, manzagop wrote: > May be worth mentioning GetNextAnalyzer calls for different pids can't be > interleaved. Better to get rid of the parameter altogether so it can't be used incorrectly. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.h:224: // A set of all known process IDs. On 2017/03/21 21:10:05, manzagop wrote: > Looks like this empties itself as we iterate over processes. Likely worth > expanding on comment. Done. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:438: // Find any new data that may have been added by an active instance of this On 2017/03/21 21:10:05, manzagop wrote: > Can you say more about what/where this other instance is? IIUC there'd need to > be some locking for the writes not to step on each other's toes below? You're right. I was once thinking they could operate independently but that's a lot more lock-free work than is necessary and would only be needed if "global data" makes a return. Note that the CL that removes "global data" depends on this one landing first. Oh well. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:584: memory_ = nullptr; On 2017/03/21 21:10:05, manzagop wrote: > This behavior should be mentioned in the .h. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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 bcwhite@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...
Patchset #5 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by bcwhite@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...
Great! lgtm https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.cc:332: // Reverse the list of PIDs so that they get popped in the order found. On 2017/03/29 22:00:51, bcwhite wrote: > On 2017/03/21 21:10:05, manzagop wrote: > > Is the order important? We don't seem to care about it for threads. Could we > > just drop the pid vector? > > There has to be some list of PIDs though it could be the (merged?) keys of any > map. > > But I think it's worth keeping the order because then the first process (i.e. > the Browser) will be the first one returned and that might be useful during > analysis. The retrieval by creation order sounds worth adding to the comments for Get*Process(). https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.h:147: // Iterates over all known valid processes and returns their PIDs or zero if On 2017/03/29 22:00:51, bcwhite wrote: > On 2017/03/21 21:10:05, manzagop wrote: > > So... can zero be a valid pid? (just checking) > > No. On windows it's a valid pid, but not one we'll have (it's the system idle process). https://codereview.chromium.org/2754483002/diff/140001/base/debug/activity_an... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2754483002/diff/140001/base/debug/activity_an... base/debug/activity_analyzer.cc:326: // Track PIDs. May be worth noting in the comment the assumption that every process has process data. https://codereview.chromium.org/2754483002/diff/140001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2754483002/diff/140001/base/debug/activity_tr... base/debug/activity_tracker.h:909: // This access to the persistent allocator is only for testing; it extracts Update comment.
The CQ bit was checked by bcwhite@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 checked by bcwhite@chromium.org to run a CQ dry run
Patchset #7 (id:180001) has been deleted
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...)
https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.cc:332: // Reverse the list of PIDs so that they get popped in the order found. On 2017/03/30 18:13:24, manzagop wrote: > On 2017/03/29 22:00:51, bcwhite wrote: > > On 2017/03/21 21:10:05, manzagop wrote: > > > Is the order important? We don't seem to care about it for threads. Could we > > > just drop the pid vector? > > > > There has to be some list of PIDs though it could be the (merged?) keys of any > > map. > > > > But I think it's worth keeping the order because then the first process (i.e. > > the Browser) will be the first one returned and that might be useful during > > analysis. > > The retrieval by creation order sounds worth adding to the comments for > Get*Process(). Done. https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/2754483002/diff/60001/base/debug/activity_ana... base/debug/activity_analyzer.h:147: // Iterates over all known valid processes and returns their PIDs or zero if On 2017/03/30 18:13:24, manzagop wrote: > On 2017/03/29 22:00:51, bcwhite wrote: > > On 2017/03/21 21:10:05, manzagop wrote: > > > So... can zero be a valid pid? (just checking) > > > > No. > > On windows it's a valid pid, but not one we'll have (it's the system idle > process). Acknowledged. https://codereview.chromium.org/2754483002/diff/140001/base/debug/activity_an... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2754483002/diff/140001/base/debug/activity_an... base/debug/activity_analyzer.cc:326: // Track PIDs. On 2017/03/30 18:13:24, manzagop wrote: > May be worth noting in the comment the assumption that every process has process > data. I had to do the add in both sections because some tests don't create the GlobalActivityTracker and thus don't have process_data. https://codereview.chromium.org/2754483002/diff/140001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2754483002/diff/140001/base/debug/activity_tr... base/debug/activity_tracker.h:909: // This access to the persistent allocator is only for testing; it extracts On 2017/03/30 18:13:24, manzagop wrote: > Update comment. Oops. :-)
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org Link to the patchset: https://codereview.chromium.org/2754483002/#ps200001 (title: "addressed review comments by manzagop")
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": 200001, "attempt_start_ts": 1490975282955100,
"parent_rev": "44c111b0f7bd9a9bb044c527162bb5bc7e6c0250", "commit_rev":
"e7aca0670b59b9062005a9b5a96cfb2d29743682"}
Message was sent while issue was closed.
Description was changed from ========== Add analyzer support for multiple processes. BUG=620813 ========== to ========== Add analyzer support for multiple processes. BUG=620813 Review-Url: https://codereview.chromium.org/2754483002 Cr-Commit-Position: refs/heads/master@{#461141} Committed: https://chromium.googlesource.com/chromium/src/+/e7aca0670b59b9062005a9b5a96c... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001) as https://chromium.googlesource.com/chromium/src/+/e7aca0670b59b9062005a9b5a96c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
