Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2178733002:
Task manager should support Idle Wakeups on Windows (Closed)
DescriptionTask manager should support Idle Wakeups on Windows
This change adds a special SharedSampler that can refresh
multiple TaskManager resources for all chrome Task Groups
in a single operation.
On OS_WIN the implementation is based on
NtQuerySystemInformation and currently provides
only Idle Wakeups per second to start with. It should be
fairly straightforward to support a few other resource
types later, for example CPU usage and Private Memory.
On non-Windows platforms SharedSampler has a trivial stub
implementation that does nothing.
BUG=620832
Committed: https://crrev.com/71b5b48907b3a310e9006a427dda3f2ffbc08bc3
Cr-Commit-Position: refs/heads/master@{#410236}
Patch Set 1 : Finished coding #Patch Set 2 : Added test #Patch Set 3 : Fixed build errors #Patch Set 4 : Use std::move and lint fixes. #Patch Set 5 : Misc fixes #Patch Set 6 : Fixed build error on win_clang. #
Total comments: 60
Patch Set 7 : Addressed most of CR feedback #Patch Set 8 : Handling missing results in OnRefreshDone. #
Total comments: 40
Patch Set 9 : Resolved merge conflicts and addressed CR feedback #Patch Set 10 : Fixed build error on win_clang #Messages
Total messages: 76 (50 generated)
The CQ bit was checked by stanisc@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by stanisc@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: 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 stanisc@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by stanisc@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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by stanisc@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_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_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_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_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 stanisc@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 #1 (id:1) has been deleted
Description was changed from ========== Added most of the logic First iteration of coding done. BUG= ========== to ========== Task manager should support Idle Wakeups on Windows This change adds a special SharedSampler that can refresh multiple TaskManager resources for all chrome Task Groups in a single operation. On OS_WIN the implementation is based on NtQuerySystemInformation and currently provides only Idle Wakeups per second to start with. It should be fairly straightforward to support a few other resource types later, for example CPU usage and Private Memory. On non-Windows platforms SharedSampler has a trivial stub implementation that does nothing. BUG=620832 ==========
stanisc@chromium.org changed reviewers: + nick@chromium.org
PTAL!
The CQ bit was checked by stanisc@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.
nick@chromium.org changed reviewers: + scottmg@chromium.org
+scottmg: can you have a look at the SYSTEM_PROCESS_INFORMATION code in shared_sampler_win.cc ? I'm wondering there's any possibility of reusing crashpad's ProcessReaderWin here? https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:508: } This is unnecessary -- see comment in gypi file. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler.cc (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.cc:11: #if !defined(OS_WIN) Could we make this file be shared_sampler_posix.cc and avoid this ifdef? https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler.h (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:28: // mainly on a blocking pool thread. PrivateWorkingSetSnapshot had the following comment; we should do something similar here to explain why we need SharedSampler: // This // exists because on Windows it is much faster to collect a group of private // working sets all at once using PdhOpenQuery than to calculate the private // working sets for each process individually. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:31: SharedSampler( single-arg ctor should be explicit https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:61: Callbacks(Callbacks&& other); could this be = default? That saves us from having to remember to update the move ctor body as new fields are added. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:77: typedef std::map<base::ProcessId, RefreshResult> RefreshResultMap; A vector would probably be faster. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:109: std::vector<std::pair<base::ProcessId, int64_t>> refresh_flags_; I don't think we really need to track the refresh flags on a per-process basis. The refresh flags are the union of the needs of all task manager observers at the moment, but every observer is listening for info about all processes. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:118: const base::FilePath current_process_image_name_; maybe this should be a vector of FilePaths, which contains both current_process_image_name and also nacl64? https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:85: struct SYSTEM_PROCESS_INFORMATION { This is also declared (slightly differently) in: third_party/crashpad/crashpad/util/win/process_structs.h Which leads me to two questions: (1) can we reuse the crashpad def? and (2) is one more correct than the other? https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:330: MakeResultsFromSnapshot(*snapshot, results.get()); Is MakeResultsFromSnapshot different from just doing MakeResultsFromTwoSnapshots, where |previous_snapshot_| is empty? https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:333: previous_snapshot_ = std::move(snapshot); If we close the TaskManager, or hide the idle wakeups column, it seems like the SharedSampler remains, and its retained |previous_snapshot_| will get arbitrarily old. Does this suggest that if we later re-show the column, we'd get an initial spike in the values, proportional to the time that the column had been invisible? https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:338: bool SharedSampler::IsChromeImageName(const wchar_t* image_name) const { Using base:: string comparison function would result in code that's more readable to non-Windows programmers. Maybe like this: bool SharedSampler::IsChromeImageName(FilePath::StringPieceType image_name) { return FilePath::CompareEqualIgnoreCase(image_name, current_process_image_name_.value()) || FilePath::CompareEqualIgnoreCase(image_name, kNacl64Exe); } https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:355: return std::unique_ptr<ProcessDataSnapshot>(); Would it be better to return an empty snapshot here, rather than a null pointer? That way, the consumer doesn't need to handle two different kinds of nothing. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:448: auto process_id = iter->first; I would use base::ProcessId instead of auto for the process_id variables. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:455: if (prev_snapshot_process_id < process_id) { The two-iterator approach would be natural if we needed to do a merge, but since we're skipping values that are in |prev_snapshot| but not |snapshot|, it seems we don't actually need a symmetric loop structure. So another option here, that might be clearer to understand, is to use an outer loop over |snapshot| paired with a helper function to advance the |prev_iter|, like this: // Helper function. Advances the given sorted range past |process_id|, and returns the // value that if there ProcessData* SeekInPreviousSnapshot(ProcessId process_id, ProcessDataMap::iterator* it_to_advance, const ProcessDataMap::iterator& range_end) { for (; *it_to_advance != range_end && (*it_to_advance)->first <= process_id; ++(*it_to_advance)) { if ((*it_to_advance)->first == process_id) { return &(((*it_to_advance)++)->second); } } return nullptr; } // ... // Loop structure. old_value_it = prev_snapshot.processes.begin(); for (auto& current_entry : snapshot.processes) { base::ProcessId process_id = current_entry.first; const ProcessData& current_value = current_entry.second; ProcessData* old_value = SeekInPreviousSnapshot( process_id, &old_value_it, prev_snapshot.processes.end()) { if (old_value) { // Take delta |current_value| - |*old_value| } else { // Take |current_value| } } } Maybe there's a fancy <algorithm> way of expressing that helper function? std::lower_bound is the operation we want, except we don't want a binary search. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:504: for (auto& process_flags : refresh_flags_) { const auto&, unless you intend to mutuate. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:509: // thread_id = 0 before the actual thread ID gets assigned. The question is whether or not to invoke the callback if no data is available? For other columns, e.g. memory, we flash N/A for the first second (wez has a CL out to change N/A to a dash, which will be less distracting. I think using a sentinel value like -1 makes sense here, so that we invoke the callbacks for each request. Having said that, I'd defer to Ahmed's judgement here, if he disagrees. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:514: auto& result = result_iter->second; This declaration seems far from its first use. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/task_group.cc (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/task_group.cc:70: const scoped_refptr<SharedSampler> shared_sampler, Passing like this will cause an extra increment/decrement of the refcount. You can eliminate that either by: - pass this as a const ref - pass as a non-const value, and std::move() it into shared_sampler_ https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/task_manager_browsertest.cc (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/task_manager_browsertest.cc:855: IdleWakeups) { I think this test is a good start. By its nature it might turn out to be flaky and have {platform|trybot|configuration}-dependent behavior, so I expect some iteration here. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/task_manager_browsertest.cc:862: "function myWait(n) {\n" I would probably make this an infinite setTimeout chain. TaskManager only samples every 1 sec so running for 1000 1ms intervals might not quite cut it -- we might be thrown awry, say, by something like an unlucky gc pause on the 999th iteration. We can rely on WaitForTaskManagerStatToExceed not running forever (it has a test timeout, unless you run it with a debugger attached); and it should still be possible to cleanly close a tab that's waking up every 1ms. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/task_manager_browsertest.cc:873: browser()->tab_strip_model()->GetActiveWebContents(), test_js, &ok)); ExecuteScriptAndExtractString will block until window.domAutomationController.send() completes, so we don't start looking at the TaskManager stats until after the js has finished running. If you move the window.domAutomationController.send('okay') so that it's after the myWait(0) call, they'll run concurrently. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_columns.cc (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_columns.cc:75: #if defined(OS_MACOSX) || defined(OS_LINUX) || defined(OS_WIN) I think this is all platforms now, so we can just remove the #ifdef? (the only missing platform is android, where the task manager isn't compiled) https://codereview.chromium.org/2178733002/diff/120001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/chrome_browser.... chrome/chrome_browser.gypi:3205: 'chrome_browser_task_manager_win_sources': [ I don't think you need this -- I think you can just put it in the chrome_browser_task_manager_sources list without any conditionals. If a file in the main list has a platform suffix like _win.cc or _posix.cc, it's supposed to be automatically excluded from other platforms. This is thanks to the common rules in: https://cs.chromium.org/chromium/src/build/filename_rules.gypi?q=_posix+file:...
On 2016/07/28 21:37:27, ncarter wrote: > +scottmg: can you have a look at the SYSTEM_PROCESS_INFORMATION code in > shared_sampler_win.cc ? I'm wondering there's any possibility of reusing > crashpad's ProcessReaderWin here? Possibly, sort of grudgingly https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/snapshot/a... was added and that uses ProcessReaderWin. Another helper in snapshot/api seems reasonable to avoid duplication with code here.
On 2016/07/28 22:05:55, scottmg (OOO until August) wrote: > On 2016/07/28 21:37:27, ncarter wrote: > > +scottmg: can you have a look at the SYSTEM_PROCESS_INFORMATION code in > > shared_sampler_win.cc ? I'm wondering there's any possibility of reusing > > crashpad's ProcessReaderWin here? > > Possibly, sort of grudgingly > https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/snapshot/a... > was added and that uses ProcessReaderWin. Another helper in snapshot/api seems > reasonable to avoid duplication with code here. Yeah, ugh. chrome's task_manager depending on crashpad is also seems like a pretty weird dependency, that might cause headaches in the future. And on second look I'm not sure if ProcessReaderWin is quite what we need for the task manager here. (stan or bruce probably would have a better view of this than me). I think the NextProcess helper function and the third_party/crashpad/crashpad/util/win/process_structs.h definitions are at least candidates for sharing. Also worth noting that https://cs.chromium.org/chromium/src/components/startup_metric_utils/browser/... defines the _EX variant of this struct, which ??. I assume base/win isn't worth contemplating as an option for process_structs.h?
Thanks for the thorough CR feedback! Some answers below. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler.h (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:28: // mainly on a blocking pool thread. On 2016/07/28 21:37:26, ncarter wrote: > PrivateWorkingSetSnapshot had the following comment; we should do something > similar here to explain why we need SharedSampler: > > // This > // exists because on Windows it is much faster to collect a group of private > // working sets all at once using PdhOpenQuery than to calculate the private > // working sets for each process individually. Good idea! https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:61: Callbacks(Callbacks&& other); On 2016/07/28 21:37:26, ncarter wrote: > could this be = default? > > That saves us from having to remember to update the move ctor body as new fields > are added. I tried '= default' but that still generated an error on win_clang. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:77: typedef std::map<base::ProcessId, RefreshResult> RefreshResultMap; On 2016/07/28 21:37:26, ncarter wrote: > A vector would probably be faster. I thought about that but then I'd have to deal with merging two vectors in OnRefreshDone - this one and the vector of per-process refresh flags. Actually, according to your comment below per-process flags aren't necessary so I could now change this into a vector. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:109: std::vector<std::pair<base::ProcessId, int64_t>> refresh_flags_; On 2016/07/28 21:37:26, ncarter wrote: > I don't think we really need to track the refresh flags on a per-process basis. > The refresh flags are the union of the needs of all task manager observers at > the moment, but every observer is listening for info about all processes. I see. I was wondering about that. So basically all consecutive Refresh calls should be made with the same flags. The implementation could simply DCHECK that. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:118: const base::FilePath current_process_image_name_; On 2016/07/28 21:37:26, ncarter wrote: > maybe this should be a vector of FilePaths, which contains both > current_process_image_name and also nacl64? Hmm... The implementation would have to iterate over an array which would be slower than the current hardcoded version. But I have no problem doing this if you think that is a good idea. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:85: struct SYSTEM_PROCESS_INFORMATION { On 2016/07/28 21:37:26, ncarter wrote: > This is also declared (slightly differently) in: > > third_party/crashpad/crashpad/util/win/process_structs.h > > Which leads me to two questions: (1) can we reuse the crashpad def? and (2) is > one more correct than the other? I saw the process_structs.h version and slight differences in the struct definitions, especially around HANDLE used here for process/thread IDs vs. padded DWORD used in the crashpad version. I wasn't sure if this code could depend on third_party/crashpad and ended up just copying this from Bruce's prototype where it was proven to work. Should process_structs.h be moved into some common location in base? https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:330: MakeResultsFromSnapshot(*snapshot, results.get()); On 2016/07/28 21:37:26, ncarter wrote: > Is MakeResultsFromSnapshot different from just doing > MakeResultsFromTwoSnapshots, where |previous_snapshot_| is empty? It is different. MakeResultsFromTwoSnapshots would report the current absolute number of context switches as CS deltas for each process. So we'd end up showing a huge CS rate. MakeResultsFromSnapshot reports zeros for each process. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:333: previous_snapshot_ = std::move(snapshot); On 2016/07/28 21:37:26, ncarter wrote: > If we close the TaskManager, or hide the idle wakeups column, it seems like the > SharedSampler remains, and its retained |previous_snapshot_| will get > arbitrarily old. Does this suggest that if we later re-show the column, we'd get > an initial spike in the values, proportional to the time that the column had > been invisible? Hmm... Yes, that is something I didn't consider. It would divide CS increments by the time delta between snapshots. So the values would be OK if not on the low side because it wouldn't discount any context switches for stopped threads. For new processes the number of CS switches averaged over time would be on the low side too. So we could keep the implementation as it. It would show low values for one second but that isn't much worse than just showing zeros. If we'd prefer to show zeros for the first second than we'd need to either (1) delete the old snapshot or (2) not use the old snapshot if the difference between timestamps is greater than some predetermined threshold interval. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:355: return std::unique_ptr<ProcessDataSnapshot>(); On 2016/07/28 21:37:26, ncarter wrote: > Would it be better to return an empty snapshot here, rather than a null pointer? > That way, the consumer doesn't need to handle two different kinds of nothing. I realized the current implementation doesn't actually handle a null snapshot. That is something I'd overlooked. I'll think about this. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:455: if (prev_snapshot_process_id < process_id) { On 2016/07/28 21:37:26, ncarter wrote: > The two-iterator approach would be natural if we needed to do a merge, but since > we're skipping values that are in |prev_snapshot| but not |snapshot|, it seems > we don't actually need a symmetric loop structure. So another option here, that > might be clearer to understand, is to use an outer loop over |snapshot| paired > with a helper function to advance the |prev_iter|, like this: > > // Helper function. Advances the given sorted range past |process_id|, and > returns the > // value that if there > ProcessData* SeekInPreviousSnapshot(ProcessId process_id, > ProcessDataMap::iterator* it_to_advance, > const ProcessDataMap::iterator& range_end) { > for (; *it_to_advance != range_end && (*it_to_advance)->first <= process_id; > ++(*it_to_advance)) { > if ((*it_to_advance)->first == process_id) { > return &(((*it_to_advance)++)->second); > } > } > return nullptr; > } > > // ... > // Loop structure. > old_value_it = prev_snapshot.processes.begin(); > for (auto& current_entry : snapshot.processes) { > base::ProcessId process_id = current_entry.first; > const ProcessData& current_value = current_entry.second; > ProcessData* old_value = SeekInPreviousSnapshot( > process_id, &old_value_it, prev_snapshot.processes.end()) { > if (old_value) { > // Take delta |current_value| - |*old_value| > } else { > // Take |current_value| > } > } > } > > Maybe there's a fancy <algorithm> way of expressing that helper function? > std::lower_bound is the operation we want, except we don't want a binary search. Good idea! I'll think about this. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/task_group.cc (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/task_group.cc:70: const scoped_refptr<SharedSampler> shared_sampler, On 2016/07/28 21:37:27, ncarter wrote: > Passing like this will cause an extra increment/decrement of the refcount. > > You can eliminate that either by: > > - pass this as a const ref > - pass as a non-const value, and std::move() it into shared_sampler_ Yeah, I meant a const ref here, but omitted '&' by mistake. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_columns.cc (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_columns.cc:75: #if defined(OS_MACOSX) || defined(OS_LINUX) || defined(OS_WIN) On 2016/07/28 21:37:27, ncarter wrote: > I think this is all platforms now, so we can just remove the #ifdef? > > (the only missing platform is android, where the task manager isn't compiled) I was wondering that but wasn't sure if this now cover all platforms. Will fix this. https://codereview.chromium.org/2178733002/diff/120001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/chrome_browser.... chrome/chrome_browser.gypi:3205: 'chrome_browser_task_manager_win_sources': [ On 2016/07/28 21:37:27, ncarter wrote: > I don't think you need this -- I think you can just put it in the > chrome_browser_task_manager_sources list without any conditionals. > > If a file in the main list has a platform suffix like _win.cc or _posix.cc, it's > supposed to be automatically excluded from other platforms. This is thanks to > the common rules in: > https://cs.chromium.org/chromium/src/build/filename_rules.gypi?q=_posix+file:... That's neat! Will fix this.
On 2016/07/28 22:28:41, ncarter wrote: > On 2016/07/28 22:05:55, scottmg (OOO until August) wrote: > > On 2016/07/28 21:37:27, ncarter wrote: > > > +scottmg: can you have a look at the SYSTEM_PROCESS_INFORMATION code in > > > shared_sampler_win.cc ? I'm wondering there's any possibility of reusing > > > crashpad's ProcessReaderWin here? > > > > Possibly, sort of grudgingly > > > https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/snapshot/a... > > was added and that uses ProcessReaderWin. Another helper in snapshot/api seems > > reasonable to avoid duplication with code here. > > Yeah, ugh. chrome's task_manager depending on crashpad is also seems like a > pretty weird dependency, that might cause headaches in the future. And on second > look I'm not sure if ProcessReaderWin is quite what we need for the task manager > here. (stan or bruce probably would have a better view of this than me). I think > the NextProcess helper function and the > third_party/crashpad/crashpad/util/win/process_structs.h definitions are at > least candidates for sharing. Insofar as it uses NtQuerySystemInformation to read SystemProcessInformation, it seems shareable. I think if we're going to have the dependency, we should make a snapshot/api wrapper rather than just directly reusing the header though. > > Also worth noting that > https://cs.chromium.org/chromium/src/components/startup_metric_utils/browser/... > defines the _EX variant of this struct, which ??. I don't think _EX is a thing. Maybe the intention was to not have it conflict with this one https://msdn.microsoft.com/en-us/library/windows/desktop/ms724509(v=vs.85).aspx ? But it's a bit confusing. It seems worthwhile sharing with at least that one. > > I assume base/win isn't worth contemplating as an option for process_structs.h? We could do that too. Unfortunately it ends up being the same number of copies as we'd then have to duplicate it into crashpad/mini_chromium (crashpad's base that it uses when not building in chrome).
scottmg@chromium.org changed reviewers: + mark@chromium.org
+mark as fyi since we've previously talked about whether/how to expose more of the internals of Crashpad (previously for the "plugin" case, but also here just for general code reuse).
scottmg wrote: > +mark as fyi since we've previously talked about whether/how to expose more of > the internals of Crashpad (previously for the "plugin" case, but also here just > for general code reuse). Yeah, I share your “possibly, sort of grudgingly” sentiment, but I do want to find a nicer solution than basically copying and pasting the same code and structs all over the place. I think that Crashpad does kind of want to own this code. And I don’t think that base is a very good home for it. But it also seems like internal implementation bits of Crashpad, and I don’t know if it would have been near the top of the list of things we’d have expected to expose to the world. If you’re familiar with linux-syscall-support, maybe it makes sense to extract the nt_internals parts of Crashpad (including these undocumented and incompletely-documented structs) in the same way. One complicating factor is that Crashpad goes the extra mile to deal with cross-bitted situations, which most code doesn’t even try to deal with because Microsoft made it pretty difficult to do so. That’s why our SYSTEM_PROCESS_INFORMATION is templatized like https://chromium.googlesource.com/crashpad/crashpad/+/0a717f0d27ffd2e84edb937.... (On Mac, I used an even more elaborate scheme, https://chromium.googlesource.com/crashpad/crashpad/+/7b8de8a40474473152482d6...) That’s great for handling cross-bitting, but it’s probably overkill for anyone else that doesn’t care. Do you care here?
And although I mentioned linux-syscall-support as an example, maybe that’s a terrible idea. Last week I had a really terrible time dealing with a tangle of DEPS dependencies, and LSS was a major part of it. https://crbug.com/629874 has all of the gross details.
The CQ bit was checked by stanisc@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) 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 stanisc@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...
I think I've addressed most issues except the duplicating NT internals data structures. I am not sure what is the best way to resolve the duplication given the other copy of data structures is under third_party/crashpad. PTAL! https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:508: } On 2016/07/28 21:37:26, ncarter wrote: > This is unnecessary -- see comment in gypi file. Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler.cc (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.cc:11: #if !defined(OS_WIN) On 2016/07/28 21:37:26, ncarter wrote: > Could we make this file be shared_sampler_posix.cc and avoid this ifdef? Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler.h (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:28: // mainly on a blocking pool thread. On 2016/07/28 22:38:43, stanisc wrote: > On 2016/07/28 21:37:26, ncarter wrote: > > PrivateWorkingSetSnapshot had the following comment; we should do something > > similar here to explain why we need SharedSampler: > > > > // This > > // exists because on Windows it is much faster to collect a group of private > > // working sets all at once using PdhOpenQuery than to calculate the private > > // working sets for each process individually. > > Good idea! Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:31: SharedSampler( On 2016/07/28 21:37:26, ncarter wrote: > single-arg ctor should be explicit Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:61: Callbacks(Callbacks&& other); On 2016/07/28 22:38:43, stanisc wrote: > On 2016/07/28 21:37:26, ncarter wrote: > > could this be = default? > > > > That saves us from having to remember to update the move ctor body as new > fields > > are added. > > I tried '= default' but that still generated an error on win_clang. See win_clang error in patchset #5: https://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds... https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:77: typedef std::map<base::ProcessId, RefreshResult> RefreshResultMap; On 2016/07/28 22:38:43, stanisc wrote: > On 2016/07/28 21:37:26, ncarter wrote: > > A vector would probably be faster. > > I thought about that but then I'd have to deal with merging two vectors in > OnRefreshDone - this one and the vector of per-process refresh flags. > > Actually, according to your comment below per-process flags aren't necessary so > I could now change this into a vector. Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:109: std::vector<std::pair<base::ProcessId, int64_t>> refresh_flags_; On 2016/07/28 22:38:43, stanisc wrote: > On 2016/07/28 21:37:26, ncarter wrote: > > I don't think we really need to track the refresh flags on a per-process > basis. > > The refresh flags are the union of the needs of all task manager observers at > > the moment, but every observer is listening for info about all processes. > > I see. I was wondering about that. So basically all consecutive Refresh calls > should be made with the same flags. The implementation could simply DCHECK that. Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:118: const base::FilePath current_process_image_name_; On 2016/07/28 21:37:26, ncarter wrote: > maybe this should be a vector of FilePaths, which contains both > current_process_image_name and also nacl64? Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:333: previous_snapshot_ = std::move(snapshot); On 2016/07/28 22:38:44, stanisc wrote: > On 2016/07/28 21:37:26, ncarter wrote: > > If we close the TaskManager, or hide the idle wakeups column, it seems like > the > > SharedSampler remains, and its retained |previous_snapshot_| will get > > arbitrarily old. Does this suggest that if we later re-show the column, we'd > get > > an initial spike in the values, proportional to the time that the column had > > been invisible? > > Hmm... Yes, that is something I didn't consider. It would divide CS increments > by the time delta between snapshots. So the values would be OK if not on the low > side because it wouldn't discount any context switches for stopped threads. For > new processes the number of CS switches averaged over time would be on the low > side too. > > So we could keep the implementation as it. It would show low values for one > second but that isn't much worse than just showing zeros. > If we'd prefer to show zeros for the first second than we'd need to either (1) > delete the old snapshot or (2) not use the old snapshot if the difference > between timestamps is greater than some predetermined threshold interval. OK, I've handled the "closing the TaskManager" case by clearing the cached snapshot when all callbacks are unregistered. I haven't addressed the case when Idle Wakeups column is removed and then added back. On the surface the numbers look fine in the second case because, as I explained above, the actual time delta between the two snapshots is used so the larger CS increment is divided by a larger time delta. If anything this first second Idle Wakeups value would be on the lower side because it would count number of context switches from any threads that terminated during this interval while the column wasn't visible. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:338: bool SharedSampler::IsChromeImageName(const wchar_t* image_name) const { On 2016/07/28 21:37:26, ncarter wrote: > Using base:: string comparison function would result in code > that's more readable to non-Windows programmers. Maybe like this: > > bool SharedSampler::IsChromeImageName(FilePath::StringPieceType image_name) { > return FilePath::CompareEqualIgnoreCase(image_name, > current_process_image_name_.value()) > || > FilePath::CompareEqualIgnoreCase(image_name, kNacl64Exe); > } Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:355: return std::unique_ptr<ProcessDataSnapshot>(); On 2016/07/28 22:38:44, stanisc wrote: > On 2016/07/28 21:37:26, ncarter wrote: > > Would it be better to return an empty snapshot here, rather than a null > pointer? > > That way, the consumer doesn't need to handle two different kinds of nothing. > > I realized the current implementation doesn't actually handle a null snapshot. > That is something I'd overlooked. > I'll think about this. I decided to keep this approach to indicate that something went wrong. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:448: auto process_id = iter->first; On 2016/07/28 21:37:27, ncarter wrote: > I would use base::ProcessId instead of auto for the process_id variables. Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:455: if (prev_snapshot_process_id < process_id) { On 2016/07/28 21:37:26, ncarter wrote: > The two-iterator approach would be natural if we needed to do a merge, but since > we're skipping values that are in |prev_snapshot| but not |snapshot|, it seems > we don't actually need a symmetric loop structure. So another option here, that > might be clearer to understand, is to use an outer loop over |snapshot| paired > with a helper function to advance the |prev_iter|, like this: > > // Helper function. Advances the given sorted range past |process_id|, and > returns the > // value that if there > ProcessData* SeekInPreviousSnapshot(ProcessId process_id, > ProcessDataMap::iterator* it_to_advance, > const ProcessDataMap::iterator& range_end) { > for (; *it_to_advance != range_end && (*it_to_advance)->first <= process_id; > ++(*it_to_advance)) { > if ((*it_to_advance)->first == process_id) { > return &(((*it_to_advance)++)->second); > } > } > return nullptr; > } > > // ... > // Loop structure. > old_value_it = prev_snapshot.processes.begin(); > for (auto& current_entry : snapshot.processes) { > base::ProcessId process_id = current_entry.first; > const ProcessData& current_value = current_entry.second; > ProcessData* old_value = SeekInPreviousSnapshot( > process_id, &old_value_it, prev_snapshot.processes.end()) { > if (old_value) { > // Take delta |current_value| - |*old_value| > } else { > // Take |current_value| > } > } > } > > Maybe there's a fancy <algorithm> way of expressing that helper function? > std::lower_bound is the operation we want, except we don't want a binary search. Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:504: for (auto& process_flags : refresh_flags_) { On 2016/07/28 21:37:27, ncarter wrote: > const auto&, unless you intend to mutuate. Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:509: // thread_id = 0 before the actual thread ID gets assigned. On 2016/07/28 21:37:26, ncarter wrote: > The question is whether or not to invoke the callback if no data is available? > For other columns, e.g. memory, we flash N/A for the first second (wez has a CL > out to change N/A to a dash, which will be less distracting. I think using a > sentinel value like -1 makes sense here, so that we invoke the callbacks for > each request. > > Having said that, I'd defer to Ahmed's judgement here, if he disagrees. OK. I ended up rewriting this whole method so the iteration goes primarily over the registered callbacks and that guarantees every callback to be invoked even if there is no result. This should also handle the case when NtQuerySystemInformation failed for whatever reason so there are no results at all. And yes, the sentinel value -1 is used in that case. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:514: auto& result = result_iter->second; On 2016/07/28 21:37:27, ncarter wrote: > This declaration seems far from its first use. Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/task_group.cc (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/task_group.cc:70: const scoped_refptr<SharedSampler> shared_sampler, On 2016/07/28 22:38:44, stanisc wrote: > On 2016/07/28 21:37:27, ncarter wrote: > > Passing like this will cause an extra increment/decrement of the refcount. > > > > You can eliminate that either by: > > > > - pass this as a const ref > > - pass as a non-const value, and std::move() it into shared_sampler_ > > Yeah, I meant a const ref here, but omitted '&' by mistake. Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/task_manager_browsertest.cc (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/task_manager_browsertest.cc:855: IdleWakeups) { On 2016/07/28 21:37:27, ncarter wrote: > I think this test is a good start. By its nature it might turn out to be flaky > and have {platform|trybot|configuration}-dependent behavior, so I expect some > iteration here. OK. I've implemented changes suggested below. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/task_manager_browsertest.cc:862: "function myWait(n) {\n" On 2016/07/28 21:37:27, ncarter wrote: > I would probably make this an infinite setTimeout chain. TaskManager only > samples every 1 sec so running for 1000 1ms intervals might not quite cut it -- > we might be thrown awry, say, by something like an unlucky gc pause on the 999th > iteration. > > We can rely on WaitForTaskManagerStatToExceed not running forever (it has a test > timeout, unless you run it with a debugger attached); and it should still be > possible to cleanly close a tab that's waking up every 1ms. Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/task_manager_browsertest.cc:873: browser()->tab_strip_model()->GetActiveWebContents(), test_js, &ok)); On 2016/07/28 21:37:27, ncarter wrote: > ExecuteScriptAndExtractString will block until > window.domAutomationController.send() completes, so we don't start looking at > the TaskManager stats until after the js has finished running. > > If you move the window.domAutomationController.send('okay') so that it's after > the myWait(0) call, they'll run concurrently. Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_columns.cc (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_columns.cc:75: #if defined(OS_MACOSX) || defined(OS_LINUX) || defined(OS_WIN) On 2016/07/28 22:38:44, stanisc wrote: > On 2016/07/28 21:37:27, ncarter wrote: > > I think this is all platforms now, so we can just remove the #ifdef? > > > > (the only missing platform is android, where the task manager isn't compiled) > > I was wondering that but wasn't sure if this now cover all platforms. Will fix > this. Done. https://codereview.chromium.org/2178733002/diff/120001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/chrome_browser.... chrome/chrome_browser.gypi:3205: 'chrome_browser_task_manager_win_sources': [ On 2016/07/28 21:37:27, ncarter wrote: > I don't think you need this -- I think you can just put it in the > chrome_browser_task_manager_sources list without any conditionals. > > If a file in the main list has a platform suffix like _win.cc or _posix.cc, it's > supposed to be automatically excluded from other platforms. This is thanks to > the common rules in: > https://cs.chromium.org/chromium/src/build/filename_rules.gypi?q=_posix+file:... Done. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:572: // |refresh_results| contains an extra entry for a process that This comment is wrong. It is the opposite - process_id is missing in |refresh_results|. I'll fix this in the next patch set.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
A few comments - mostly spelling type. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler.h (right): https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:27: // on the worker thread. Created by TaskManagerImpl the UI thread, but used Missing "on" before "the UI thread" https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:32: // than to query the same data for for each process individually. and because some types of data can only be collected this way. ?? https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:39: // corresponding refresh are done on the worker thread. I can't quite parse this sentence. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:85: struct SYSTEM_PROCESS_INFORMATION { If this version isn't merged with other versions then it (and SYSTEM_THREAD_INFORMATION, and any other types that might be used elsewhere) should be renamed to avoid ODR violations. Perhaps append _CHROME to the end? https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:224: // threads that don't exist anymore. Maybe tweak this comment to clarify that you're not iterating over *all* of the process threads from the previous snapshot - just up to the first one whose TID is >= new_thread.thread_id. Otherwise it looks like an O(n^2) algorithm when it's actually linear. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:319: // A group of consecuitive Refresh calls should all specify the same consecuitive -> consecutive https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:391: std::vector<BYTE> data_buffer(previous_buffer_size_); This may be premature optimization, but std::vector, while very convenient, is not actually ideal. std::vector zeroes the array of bytes in its constructor, which is unnecessary in this one case. std::vector copies the existing data when you resize, which is unnecessary in this one case. It would be easy enough to write a little class that would allocate an uninitialized array of bytes, and on resize would just free it and reallocate another array of bytes (instead of copying). I'm not sure if it's worth it. The redundant zeroing will happen every time but isn't *that* expensive, and the unnecessary copying would be rare. But, just wanted to throw it out there. I got curious as to how much code it would actually take and the (untested!) results are below. FWIW. class NotVector { NotVector(size_t size) { size_ = size; buffer_ = new BYTE[size]; } ~NotVector() { delete [] buffer_; } void resize(size_t size) { delete [] buffer_; size_ = size; buffer_ = new BYTE[size]; } size_t size() { return size_; } BYTE* data() { return buffer_; } BYTE& operator[](size_t index) { return buffer_[index]; } DISALLOW_COPY_AND_ASSIGN(NotVector); private: BYTE* buffer_; size_t size_; }; https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:459: DCHECK(inserted); Consider this syntax instead? snapshot->processes[process_id] = std::move(process_data); Ditto for the other use of .insert. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_columns.cc (left): https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_columns.cc:77: // and MacOS (http://crbug.com/120488). I think the #if needs to be retained, just modified, due to OS_ANDROID, OS_CHROMEOS, etc.
https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_columns.cc (left): https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_columns.cc:77: // and MacOS (http://crbug.com/120488). On 2016/08/03 00:36:55, brucedawson wrote: > I think the #if needs to be retained, just modified, due to OS_ANDROID, > OS_CHROMEOS, etc. Initially I retained this #if but Nick commented that it was no longer necessary. As I understand this code doesn't compile for Android and Chrome OS is covered by OS_LINUX. In fact Idle Wakeups column does show on my ChromeBook.
this looks pretty close. https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler.h (right): https://codereview.chromium.org/2178733002/diff/120001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:61: Callbacks(Callbacks&& other); On 2016/08/01 22:34:26, stanisc wrote: > On 2016/07/28 22:38:43, stanisc wrote: > > On 2016/07/28 21:37:26, ncarter wrote: > > > could this be = default? > > > > > > That saves us from having to remember to update the move ctor body as new > > fields > > > are added. > > > > I tried '= default' but that still generated an error on win_clang. > > See win_clang error in patchset #5: > https://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds... OK. Thanks for trying. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler.h (right): https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:43: int64_t SupportsFlags() const; Rename this to GetSupportedFlags() or something similar, so that it starts with a verb. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:84: // http://undocumented.ntinternals.net/index.html?page=UserMode%2FUndocumented%2... Without objection, let's leave these definitions here, and trying to address possible unification with crashpad as a subsequent CL. I think the review and discussion will be much easier if we proceed that way, since it seems like there's a range of considerations, and it's not 100% clear that sharing is actually the best approach. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:85: struct SYSTEM_PROCESS_INFORMATION { On 2016/08/03 00:36:55, brucedawson wrote: > If this version isn't merged with other versions then it (and > SYSTEM_THREAD_INFORMATION, and any other types that might be used elsewhere) > should be renamed to avoid ODR violations. Perhaps append _CHROME to the end? It's in an anonymous namespace, so I think there's no ODR worry. Right? https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:147: // processes being creating. Try a few times before giving up. "being creating" -> "being created" https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:175: // This structure is accessed only on the worker thread. Reflow this comment paragraph. (if you happen to use Sublime Text, the "Wrap Plus" package is really good at this). https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:183: // This structure is accessed only on the worker thread. Reflow this comment paragraph. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:391: std::vector<BYTE> data_buffer(previous_buffer_size_); On 2016/08/03 00:36:55, brucedawson wrote: > This may be premature optimization, but std::vector, while very convenient, is > not actually ideal. > > std::vector zeroes the array of bytes in its constructor, which is unnecessary > in this one case. > std::vector copies the existing data when you resize, which is unnecessary in > this one case. > > It would be easy enough to write a little class that would allocate an > uninitialized array of bytes, and on resize would just free it and reallocate > another array of bytes (instead of copying). I'm not sure if it's worth it. The > redundant zeroing will happen every time but isn't *that* expensive, and the > unnecessary copying would be rare. But, just wanted to throw it out there. I got > curious as to how much code it would actually take and the (untested!) results > are below. FWIW. > > class NotVector { > NotVector(size_t size) { > size_ = size; > buffer_ = new BYTE[size]; > } > ~NotVector() { > delete [] buffer_; > } > > void resize(size_t size) { > delete [] buffer_; > size_ = size; > buffer_ = new BYTE[size]; > } > > size_t size() { > return size_; > } > > BYTE* data() { > return buffer_; > } > > BYTE& operator[](size_t index) { > return buffer_[index]; > } > > DISALLOW_COPY_AND_ASSIGN(NotVector); > > private: > BYTE* buffer_; > size_t size_; > }; Use unique_ptr<BYTE[]> -- possibly wrapped in a class like bruce suggests -- if vector doesn't cut it. I'm okay with vector for now; let's let any optimizations we do be guided by profiling data. We could probably learn a lot by adding TRACE_EVENT macros to all the worker-thread refresh operations in the task manager. We do a lot of operations already each refresh that are O(#processes) and I doubt that an extra memzero will be noticable; I would wager that changing map<pid, ?>'s to hash maps, or preserving buffers across refresh cycles, would result in more measurable wins. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:496: .InMillisecondsF() / 1000; You can do .InSecondsF() instead of .InMillisecondsF() / 1000. Or you can just leave it as a TimeDelta, and do the computation in fixed point, like this: base::TimeDelta time_delta = snapshot.timestamp - prev_snapshot.timestamp; result.idle_wakeups_per_second = idle_wakeups_delta * base::TimeDelta::FromSeconds(1) / time_delta; https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:519: idle_wakeups_delta = CountContextSwitches(process); CountContextSwitches(x) seems equivalent to CountContextSwitchesDelta(ProcessData(), x) If you want to, this could let us get rid of CountContextSwitches. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... File chrome/browser/task_management/task_manager_browsertest_util.cc (right): https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/task_manager_browsertest_util.cc:116: return "Idle wake ups"; FYI: You'll hit some conflicts when you sync (avi has been busy deleting the old task_manager). avi is aware of this change, and is going to wait until you land it before doing the final step of moving chrome/browser/task_management back to chrome/browser/task_manager.
https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler.h (right): https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:24: struct ProcessDataSnapshot; Can ProcessDataSnapshot move into SharedSampler? https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:213: ULONG CountContextSwitchesDelta(const ProcessData& prev_process_data, The following definitions ought to be in an anonymous namespace if possible (to prevent possible ODR conflicts): ThreadData ProcessData ProcessDataMap CountContextSwitchesDelta CountContextSwitches SeekInPreviousSnapshot
On 2016/08/03 22:39:43, ncarter wrote: > https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... > chrome/browser/task_management/sampling/shared_sampler.h:24: struct > ProcessDataSnapshot; > Can ProcessDataSnapshot move into SharedSampler? > The reason I did it this way is because I wanted to avoid revealing ProcessDataSnapshot definition and definition of all other structures it depends on in the header file. If I move it into SharedSampler I'll have to define it there and then define everything else, right? One of the reasons I wanted to keep these structures hidden is that there are some Windows specific types. I kept it outside of the anonymous namespace for the same reason. I guess I could either move everything inside the class or introduce some nested non-anonymous namespace. What is the best way to handle this?
On 2016/08/03 22:39:43, ncarter wrote: > https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... > chrome/browser/task_management/sampling/shared_sampler.h:24: struct > ProcessDataSnapshot; > Can ProcessDataSnapshot move into SharedSampler? > The reason I did it this way is because I wanted to avoid revealing ProcessDataSnapshot definition and definition of all other structures it depends on in the header file. If I move it into SharedSampler I'll have to define it there and then define everything else, right? One of the reasons I wanted to keep these structures hidden is that there are some Windows specific types. I kept it outside of the anonymous namespace for the same reason. I guess I could either move everything inside the class or introduce some nested non-anonymous namespace. What is the best way to handle this?
Addressed Bruce's and Nick's comments. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler.h (right): https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:24: struct ProcessDataSnapshot; On 2016/08/03 22:39:43, ncarter wrote: > Can ProcessDataSnapshot move into SharedSampler? If I move into SharedSampler I'll have to define it there and also define all other structures it depends on. Some of those structures currently contain windows specific types which would also require <windows.h> to be included here. That all combined is the reason I wanted to avoid revealing all those implementation details here. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:27: // on the worker thread. Created by TaskManagerImpl the UI thread, but used On 2016/08/03 00:36:55, brucedawson wrote: > Missing "on" before "the UI thread" Done. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:32: // than to query the same data for for each process individually. On 2016/08/03 00:36:55, brucedawson wrote: > and because some types of data can only be collected this way. > > ?? Thanks, this is a good suggestion. Done. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:39: // corresponding refresh are done on the worker thread. On 2016/08/03 00:36:55, brucedawson wrote: > I can't quite parse this sentence. Well, I copied this from TaskGroupSampler.h but apparently didn't read it. Fixed. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler.h:43: int64_t SupportsFlags() const; On 2016/08/03 22:28:55, ncarter wrote: > Rename this to GetSupportedFlags() or something similar, so that it starts with > a verb. Done. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... File chrome/browser/task_management/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:85: struct SYSTEM_PROCESS_INFORMATION { On 2016/08/03 00:36:55, brucedawson wrote: > If this version isn't merged with other versions then it (and > SYSTEM_THREAD_INFORMATION, and any other types that might be used elsewhere) > should be renamed to avoid ODR violations. Perhaps append _CHROME to the end? I thought ODR would be avoided by placing this into anonymous namespace. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:147: // processes being creating. Try a few times before giving up. On 2016/08/03 22:28:55, ncarter wrote: > "being creating" -> "being created" Done. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:175: // This structure is accessed only on the worker thread. On 2016/08/03 22:28:55, ncarter wrote: > Reflow this comment paragraph. (if you happen to use Sublime Text, the "Wrap > Plus" package is really good at this). Done. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:183: // This structure is accessed only on the worker thread. On 2016/08/03 22:28:55, ncarter wrote: > Reflow this comment paragraph. Done. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:213: ULONG CountContextSwitchesDelta(const ProcessData& prev_process_data, On 2016/08/03 22:39:43, ncarter wrote: > The following definitions ought to be in an anonymous namespace if possible (to > prevent possible ODR conflicts): > > ThreadData > ProcessData > ProcessDataMap > CountContextSwitchesDelta > CountContextSwitches > SeekInPreviousSnapshot OK. Moved everything to the anonymous namespace. Deleted CountContextSwitches. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:224: // threads that don't exist anymore. On 2016/08/03 00:36:55, brucedawson wrote: > Maybe tweak this comment to clarify that you're not iterating over *all* of the > process threads from the previous snapshot - just up to the first one whose TID > is >= new_thread.thread_id. Otherwise it looks like an O(n^2) algorithm when > it's actually linear. Done. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:319: // A group of consecuitive Refresh calls should all specify the same On 2016/08/03 00:36:55, brucedawson wrote: > consecuitive -> consecutive Done. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:391: std::vector<BYTE> data_buffer(previous_buffer_size_); On 2016/08/03 00:36:55, brucedawson wrote: > This may be premature optimization, but std::vector, while very convenient, is > not actually ideal. > > std::vector zeroes the array of bytes in its constructor, which is unnecessary > in this one case. > std::vector copies the existing data when you resize, which is unnecessary in > this one case. > > It would be easy enough to write a little class that would allocate an > uninitialized array of bytes, and on resize would just free it and reallocate > another array of bytes (instead of copying). I'm not sure if it's worth it. The > redundant zeroing will happen every time but isn't *that* expensive, and the > unnecessary copying would be rare. But, just wanted to throw it out there. I got > curious as to how much code it would actually take and the (untested!) results > are below. FWIW. > > class NotVector { > NotVector(size_t size) { > size_ = size; > buffer_ = new BYTE[size]; > } > ~NotVector() { > delete [] buffer_; > } > > void resize(size_t size) { > delete [] buffer_; > size_ = size; > buffer_ = new BYTE[size]; > } > > size_t size() { > return size_; > } > > BYTE* data() { > return buffer_; > } > > BYTE& operator[](size_t index) { > return buffer_[index]; > } > > DISALLOW_COPY_AND_ASSIGN(NotVector); > > private: > BYTE* buffer_; > size_t size_; > }; OK. I've implemented something similar. I liked the idea because it allowed me to encapsulate the data size inside this class in addition to the data buffer and the buffer size. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:459: DCHECK(inserted); On 2016/08/03 00:36:55, brucedawson wrote: > Consider this syntax instead? > > snapshot->processes[process_id] = std::move(process_data); > > Ditto for the other use of .insert. The operator[] version doesn't seem to work with DISALLOW_COPY_AND_ASSIGN. The compiler complained that it couldn't access the private assignment operator. I think I should keep insert even though I agree it is less readable. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:496: .InMillisecondsF() / 1000; On 2016/08/03 22:28:55, ncarter wrote: > You can do .InSecondsF() instead of .InMillisecondsF() / 1000. Or you can just > leave it as a TimeDelta, and do the computation in fixed point, like this: > > base::TimeDelta time_delta = snapshot.timestamp - prev_snapshot.timestamp; > result.idle_wakeups_per_second = > idle_wakeups_delta * base::TimeDelta::FromSeconds(1) / time_delta; Somehow I didn't see InSecondsF() when I looked at TimeDelta methods. Fixed. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:519: idle_wakeups_delta = CountContextSwitches(process); On 2016/08/03 22:28:55, ncarter wrote: > CountContextSwitches(x) > > seems equivalent to > > CountContextSwitchesDelta(ProcessData(), x) > > If you want to, this could let us get rid of CountContextSwitches. Indeed. Done. https://codereview.chromium.org/2178733002/diff/160001/chrome/browser/task_ma... chrome/browser/task_management/sampling/shared_sampler_win.cc:572: // |refresh_results| contains an extra entry for a process that On 2016/08/01 22:34:27, stanisc wrote: > This comment is wrong. It is the opposite - process_id is missing in > |refresh_results|. > I'll fix this in the next patch set. Done.
lgtm
The CQ bit was checked by nick@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
FYI you totally need to rebase. chrome/browser/ui/task_manager is gone.
On 2016/08/04 18:22:17, Avi wrote: > FYI you totally need to rebase. chrome/browser/ui/task_manager is gone. My mistake, sorry. Never mind, chrome/browser/ui/task_manager is still there.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_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 stanisc@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.
The CQ bit was checked by stanisc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/2178733002/#ps200001 (title: "Fixed build error on win_clang")
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 ========== Task manager should support Idle Wakeups on Windows This change adds a special SharedSampler that can refresh multiple TaskManager resources for all chrome Task Groups in a single operation. On OS_WIN the implementation is based on NtQuerySystemInformation and currently provides only Idle Wakeups per second to start with. It should be fairly straightforward to support a few other resource types later, for example CPU usage and Private Memory. On non-Windows platforms SharedSampler has a trivial stub implementation that does nothing. BUG=620832 ========== to ========== Task manager should support Idle Wakeups on Windows This change adds a special SharedSampler that can refresh multiple TaskManager resources for all chrome Task Groups in a single operation. On OS_WIN the implementation is based on NtQuerySystemInformation and currently provides only Idle Wakeups per second to start with. It should be fairly straightforward to support a few other resource types later, for example CPU usage and Private Memory. On non-Windows platforms SharedSampler has a trivial stub implementation that does nothing. BUG=620832 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Task manager should support Idle Wakeups on Windows This change adds a special SharedSampler that can refresh multiple TaskManager resources for all chrome Task Groups in a single operation. On OS_WIN the implementation is based on NtQuerySystemInformation and currently provides only Idle Wakeups per second to start with. It should be fairly straightforward to support a few other resource types later, for example CPU usage and Private Memory. On non-Windows platforms SharedSampler has a trivial stub implementation that does nothing. BUG=620832 ========== to ========== Task manager should support Idle Wakeups on Windows This change adds a special SharedSampler that can refresh multiple TaskManager resources for all chrome Task Groups in a single operation. On OS_WIN the implementation is based on NtQuerySystemInformation and currently provides only Idle Wakeups per second to start with. It should be fairly straightforward to support a few other resource types later, for example CPU usage and Private Memory. On non-Windows platforms SharedSampler has a trivial stub implementation that does nothing. BUG=620832 Committed: https://crrev.com/71b5b48907b3a310e9006a427dda3f2ffbc08bc3 Cr-Commit-Position: refs/heads/master@{#410236} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/71b5b48907b3a310e9006a427dda3f2ffbc08bc3 Cr-Commit-Position: refs/heads/master@{#410236} |
