Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2573183002:
Add process start time and CPU time columns to task manager (Closed)
DescriptionAdd process start time and CPU time columns to task manager
Both start time and CPU time are obtained using shared_sampler_win.cc.
By default, they won't show up.
BUG=670921
Review-Url: https://codereview.chromium.org/2573183002
Cr-Commit-Position: refs/heads/master@{#442449}
Committed: https://chromium.googlesource.com/chromium/src/+/848410dc4e71fa3ce9727478ab013a8948c4dfc6
Patch Set 1 #Patch Set 2 : Fix nits #
Total comments: 30
Patch Set 3 : Address the questions from CR #Patch Set 4 : Update variable names #Patch Set 5 : Fix shared_sampler_unittest_win #Patch Set 6 : Add TODO comment for a bug, which will be addressed in another CL. #
Total comments: 25
Patch Set 7 : Addess CR questions #
Total comments: 4
Patch Set 8 : Move ticks-Time and ticks-TimeDelta conversions to shared_sampler_win.cc #
Total comments: 8
Patch Set 9 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into updatetaskmanager #Patch Set 10 : Fix nits and conflicts. #Messages
Total messages: 114 (91 generated)
Description was changed from ========== Format code Add CpuTime to task manager BUG=670921 ========== to ========== Add cpu time usage and process start time to task manager BUG=670921 ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #6 (id:140001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #2 (id:260001) has been deleted
Patchset #2 (id:280001) has been deleted
Patchset #2 (id:300001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_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 chengx@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:240001) has been deleted
Patchset #1 (id:320001) has been deleted
Description was changed from ========== Add cpu time usage and process start time to task manager BUG=670921 ========== to ========== Add process start time and CPU time columns to task manager BUG=670921 ==========
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_...)
The CQ bit was checked by chengx@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.
Patchset #2 (id:360001) has been deleted
The CQ bit was checked by chengx@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 #2 (id:380001) has been deleted
Description was changed from ========== Add process start time and CPU time columns to task manager BUG=670921 ========== to ========== Add process start time and CPU time columns to task manager Both start time and CPU time are obtained using shared_sampler_win.cc, and show up in task manager by default. BUG=670921 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chengx@chromium.org changed reviewers: + brucedawson@chromium.org, stanisc@chromium.org
PTAL~
> show up in task manager by default. I suspect we don't want the new columns on by default. Or, at least, that is a much bigger decision than having the columns available. https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... base/i18n/time_formatting.cc:178: const int64_t total_seconds = time.InSeconds(); Consider rounding to nearest as in TimeDurationFormat. Doesn't make much difference of course, but seems consistent. https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... base/i18n/time_formatting.cc:179: int hours = total_seconds / 3600; Why no 'const' here? It seems inconsistent with total_seconds. I am a fan of marking all variables with const if they can be, although not all share my enthusiasm. And yes, I notice that TimeDurationFormat also has this inconsistency. Is it required by icu somehow? https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... base/i18n/time_formatting.h:87: TimeDurationWFormatWithSecondPrecison(const TimeDelta& time, Why 'W'Format? Also, consider FormatWithSeconds instead of FormatWithSecondPrecision. Just my opinion though, so feel free to ignore or let Stan decide. https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... base/i18n/time_formatting_unittest.cc:274: EXPECT_EQ(ASCIIToUTF16("15 hours, 42 minutes, 30 seconds"), Should add some tests for times like 15 hours, 0 minutes, 10 seconds, and 15 hours, 10 minutes, 0 seconds. Those cases sometimes trigger errors. https://codereview.chromium.org/2573183002/diff/400001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/app/generated_r... chrome/app/generated_resources.grd:3357: + <message name="IDS_TASK_MANAGER_CPU_TIME_COLUMN" desc="The text of the CPU time usage column"> When is this text displayed? I can't figure out how to display it so I don't know how important it is that the description is not very descriptive. Unless we are certain that the text is never shown it would be worth writing something more descriptive, something to tell a reader what this column means. https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/shared_sampler.h (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler.h:92: int64_t start_time; Should these be using a type that includes semantic information about units? It's not clear what the units of start_time or cpu_time are. Right now these need a comment, but using an appropriate type could avoid that. It looks like these are using the wacky Windows 100 ns units, which is fine, but does require a comment. Also should consider using uint64_t to avoid casts - it's equivalent to ULONGLONG. https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler_win.cc:330: on_cpu_time = std::move(other.on_cpu_time); Why is std::move being used here? It offers no value for types with no owned resources. https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler_win.cc:499: process_data.start_time = static_cast<int64_t>(pi->CreateTime); What happens if you use uint64_t for start_time and cpu_time instead of int64_t? That should avoid the need for the casts. https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:100: if (time_in_100ns < 0) If you do change to unsigned type then this check will have to be for == 0. https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager_observer.h (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_observer.h:28: REFRESH_TYPE_START_TIME = 1 << 1, Why did you insert these at the beginning? Wouldn't it be simpler and easier to review if you added them at the end, to avoid renumbering everything?
https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... base/i18n/time_formatting.h:87: TimeDurationWFormatWithSecondPrecison(const TimeDelta& time, On 2016/12/19 21:51:05, brucedawson wrote: > Why 'W'Format? > > Also, consider FormatWithSeconds instead of FormatWithSecondPrecision. Just my > opinion though, so feel free to ignore or let Stan decide. What about sub-second precision? Should we consider that? https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... base/i18n/time_formatting_unittest.cc:280: EXPECT_EQ(ASCIIToUTF16("15:42:30"), TimeDurationWFormatWithSecondPrecison( Should add tests for times with greater than 24 or greater than 100 hours. https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:202: // 7- Start time. This is misleading because TaskGroupSampler doesn't yet handle Start time and CPU time. https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:99: int64_t time_in_100ns = GetTaskGroupByTaskId(task_id)->start_time(); time_in_100ns is *ticks* https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_columns.cc (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_columns.cc:44: arraysize("15h 42m 30s") * kCharWidth, -1, true, false, true }, This might be more than 100 hours. I suggest 999h instead. Also, is this the CPU time format we are using? I thought it was compact format.
Patchset #3 (id:420001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add process start time and CPU time columns to task manager Both start time and CPU time are obtained using shared_sampler_win.cc, and show up in task manager by default. BUG=670921 ========== to ========== Add process start time and CPU time columns to task manager Both start time and CPU time are obtained using shared_sampler_win.cc. By default, they won't show up. BUG=670921 ==========
On 2016/12/19 21:51:06, brucedawson wrote: > > show up in task manager by default. > > I suspect we don't want the new columns on by default. Or, at least, that is a > much bigger decision than having the columns available. Alright, I will disable showing them by default. https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... base/i18n/time_formatting.cc:178: const int64_t total_seconds = time.InSeconds(); On 2016/12/19 21:51:05, brucedawson wrote: > Consider rounding to nearest as in TimeDurationFormat. Doesn't make much > difference of course, but seems consistent. Done. https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... base/i18n/time_formatting.cc:179: int hours = total_seconds / 3600; On 2016/12/19 21:51:05, brucedawson wrote: > Why no 'const' here? It seems inconsistent with total_seconds. I am a fan of > marking all variables with const if they can be, although not all share my > enthusiasm. > > And yes, I notice that TimeDurationFormat also has this inconsistency. Is it > required by icu somehow? In icu::Measure, actually its first parameter is const, so I will just change all of them to const. https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... base/i18n/time_formatting.h:87: TimeDurationWFormatWithSecondPrecison(const TimeDelta& time, On 2016/12/19 21:51:05, brucedawson wrote: > Why 'W'Format? > > Also, consider FormatWithSeconds instead of FormatWithSecondPrecision. Just my > opinion though, so feel free to ignore or let Stan decide. Done. https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... base/i18n/time_formatting.h:87: TimeDurationWFormatWithSecondPrecison(const TimeDelta& time, On 2016/12/19 22:44:10, stanisc wrote: > On 2016/12/19 21:51:05, brucedawson wrote: > > Why 'W'Format? > > > > Also, consider FormatWithSeconds instead of FormatWithSecondPrecision. Just my > > opinion though, so feel free to ignore or let Stan decide. > > What about sub-second precision? Should we consider that? I vote for "WithSeconds", as the other one is "WithMinutes" basically though it is not mentioned in the function name. https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... base/i18n/time_formatting_unittest.cc:274: EXPECT_EQ(ASCIIToUTF16("15 hours, 42 minutes, 30 seconds"), On 2016/12/19 21:51:05, brucedawson wrote: > Should add some tests for times like 15 hours, 0 minutes, 10 seconds, and 15 > hours, 10 minutes, 0 seconds. Those cases sometimes trigger errors. Done. https://codereview.chromium.org/2573183002/diff/400001/base/i18n/time_formatt... base/i18n/time_formatting_unittest.cc:280: EXPECT_EQ(ASCIIToUTF16("15:42:30"), TimeDurationWFormatWithSecondPrecison( On 2016/12/19 22:44:10, stanisc wrote: > Should add tests for times with greater than 24 or greater than 100 hours. Actually we have a bug in //base that when the hour is greater than 24 and the format is DURATION_WIDTH_NARROW (which is the compact), the hour will be converted to its value mod by 24. I will fix it in another CL. https://codereview.chromium.org/2573183002/diff/400001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/app/generated_r... chrome/app/generated_resources.grd:3357: + <message name="IDS_TASK_MANAGER_CPU_TIME_COLUMN" desc="The text of the CPU time usage column"> On 2016/12/19 21:51:05, brucedawson wrote: > When is this text displayed? I can't figure out how to display it so I don't > know how important it is that the description is not very descriptive. > > Unless we are certain that the text is never shown it would be worth writing > something more descriptive, something to tell a reader what this column means. Done. https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/shared_sampler.h (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler.h:92: int64_t start_time; On 2016/12/19 21:51:05, brucedawson wrote: > Should these be using a type that includes semantic information about units? > It's not clear what the units of start_time or cpu_time are. Right now these > need a comment, but using an appropriate type could avoid that. > > It looks like these are using the wacky Windows 100 ns units, which is fine, but > does require a comment. Also should consider using uint64_t to avoid casts - > it's equivalent to ULONGLONG. Done. https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler_win.cc:499: process_data.start_time = static_cast<int64_t>(pi->CreateTime); On 2016/12/19 21:51:05, brucedawson wrote: > What happens if you use uint64_t for start_time and cpu_time instead of int64_t? > That should avoid the need for the casts. Done. https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:202: // 7- Start time. On 2016/12/19 22:44:10, stanisc wrote: > This is misleading because TaskGroupSampler doesn't yet handle Start time and > CPU time. I reverted the change. https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:99: int64_t time_in_100ns = GetTaskGroupByTaskId(task_id)->start_time(); On 2016/12/19 22:44:10, stanisc wrote: > time_in_100ns is *ticks* Cool. Good to know! https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:100: if (time_in_100ns < 0) On 2016/12/19 21:51:05, brucedawson wrote: > If you do change to unsigned type then this check will have to be for == 0. Done. https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager_observer.h (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_observer.h:28: REFRESH_TYPE_START_TIME = 1 << 1, On 2016/12/19 21:51:06, brucedawson wrote: > Why did you insert these at the beginning? Wouldn't it be simpler and easier to > review if you added them at the end, to avoid renumbering everything? Done. https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_columns.cc (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_columns.cc:44: arraysize("15h 42m 30s") * kCharWidth, -1, true, false, true }, On 2016/12/19 22:44:10, stanisc wrote: > This might be more than 100 hours. I suggest 999h instead. > Also, is this the CPU time format we are using? I thought it was compact format. Good point. I will 1234h just in case. Actually, we have a bug with the compact format when hour is larger than 24, which is be converted to the value mod by 24. I will fix it in another CL. So for now, I will stay with "15h 42m 30s" format.
https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:99: int64_t time_in_100ns = GetTaskGroupByTaskId(task_id)->start_time(); On 2016/12/19 22:44:10, stanisc wrote: > time_in_100ns is *ticks* Well, Microsoft also refers to a 1 ms interval as a tick (GetTickCount()) so I wouldn't use that nomenclature.
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_...)
The CQ bit was checked by chengx@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 chengx@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 #4 (id:460001) has been deleted
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by chengx@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 chengx@chromium.org to run a CQ dry run
Patchset #5 (id:500001) 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 checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/20 01:27:57, brucedawson wrote: > https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... > File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): > > https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... > chrome/browser/task_manager/sampling/task_manager_impl.cc:99: int64_t > time_in_100ns = GetTaskGroupByTaskId(task_id)->start_time(); > On 2016/12/19 22:44:10, stanisc wrote: > > time_in_100ns is *ticks* > > Well, Microsoft also refers to a 1 ms interval as a tick (GetTickCount()) so I > wouldn't use that nomenclature. Alright, back to 100 ns.
On 2016/12/20 01:27:57, brucedawson wrote: > https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... > File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): > > https://codereview.chromium.org/2573183002/diff/400001/chrome/browser/task_ma... > chrome/browser/task_manager/sampling/task_manager_impl.cc:99: int64_t > time_in_100ns = GetTaskGroupByTaskId(task_id)->start_time(); > On 2016/12/19 22:44:10, stanisc wrote: > > time_in_100ns is *ticks* > > Well, Microsoft also refers to a 1 ms interval as a tick (GetTickCount()) so I > wouldn't use that nomenclature. Alright, back to 100 ns.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chengx@chromium.org changed reviewers: + afakhry@chromium.org, dcheng@chromium.org, nick@chromium.org
Hi, this CL is ready for ready. dcheng@, can you PTAL the //base code? nick@, afakhry@, can one of you PTAL the /chrome/browser/task_manager related code? Thanks!
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with minor nits https://codereview.chromium.org/2573183002/diff/540001/base/i18n/time_formatt... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2573183002/diff/540001/base/i18n/time_formatt... base/i18n/time_formatting.h:85: // (http://crbug.com/675791) Maybe skip these comments. https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/shared_sampler.h (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler.h:92: uint64_t start_time; // in unit of 100 ns Should probably be "In units of 100 ns" (capitalization and pluralize 'units') Or "Units of 100 ns" https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler_win.cc:499: process_data.start_time = (uint64_t)pi->CreateTime; Should remove those casts - ULONGLONG and uint64_t are the same, and C style casts aren't allowed. https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler_win.cc:501: process_data.cpu_time = (uint64_t)(pi->KernelTime + pi->UserTime); Ditto.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Also please wait until nick@chromium.org reviews shared_sampler_*_win.cc. https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:148: expected_on_bg_done_flags_ = refresh_flags & kBackgroundRefreshTypesMask; Now that we have calculations done only by the |shared_sampler_| and not by the |worker_thread_sampler_|. This means for non-Windows platforms OnBackgroundRefreshTypeFinished() will never be called for the start and CPU times. If someone enables these refresh types by mistake, observers will stop receiving OnTasksRefreshedWithBackgroundCalculations() signals. Here's a suggestion: We can add TaskGroupSampler::GetSupportedFlags(). Then we'll have: int64_t expected_shared_flags = refresh_flags & shared_sampler_->GetSupportedFlags(); int64_t expected_non_shared_flags = refresh_flags & worker_thread_sampler_->GetSupportedFlags(); expected_on_bg_done_flags_ = expected_shared_flags | expected_non_shared_flags; https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.h (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.h:153: uint64_t cpu_time_; I think the units need to be documented here as well, and mention they're only calculated on Windows at the moment. https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager_interface.h (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_interface.h:80: virtual base::TimeDelta GetCpuTime(TaskId task_id) const = 0; Add a comment that these are only implemented for Windows.
lgtm with a few suggestions https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/shared_sampler.h (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler.h:55: const OnCpuTimeCallback& on_cpu_time); If we add any more callbacks we should consider replacing all this with a struct and pass a struct instead. Actually there is already a struct 'Callbacks' defined in .cc file. May be we should expose it here. https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler_win.cc:611: // Task manager will use this to display 'N/A'. Could you replace 'N/A' with '-'? https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_table_model.cc:138: base::DURATION_WIDTH_NARROW); Why not DURATION_WIDTH_NUMERIC?
I'm a little sad that we're adding fairly one-off functions into //base. TimeDurationFormat is used by two non-test callers, and TimeDurationFormatWithSeconds is used by one. I looked at some of the other things in time_formatting.h, and many are only used in one or two (non-test) locations. I can see some value in defining these things in a common place, but the layering here just doesn't seem quite right. https://codereview.chromium.org/2573183002/diff/540001/base/i18n/time_formatt... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2573183002/diff/540001/base/i18n/time_formatt... base/i18n/time_formatting.h:84: // TODO (chengx) fix function output when width = DURATION_WIDTH_NUMERIC Nit: TODO(chengx): fix function output... is the proper formatting. https://codereview.chromium.org/2573183002/diff/540001/base/i18n/time_formatt... base/i18n/time_formatting.h:99: TimeDurationFormatWithSeconds(const TimeDelta& time, Note that base::Time, base::TimeDelta, base::TimeTicks are designed to be passed by value. We should do that here.
Patchset #7 (id:560001) has been deleted
Hi, this patch addresses the questions from the previous patch. PTAL~ On 2016/12/21 20:45:37, dcheng wrote: > I'm a little sad that we're adding fairly one-off functions into //base. > TimeDurationFormat is used by two non-test callers, and > TimeDurationFormatWithSeconds is used by one. I looked at some of the other > things in time_formatting.h, and many are only used in one or two (non-test) > locations. > > I can see some value in defining these things in a common place, but the > layering here just doesn't seem quite right. I am not quite clear about the history of the time formatting code, but I do agree with you that the TimeDuration* function is only used for very few times. In addition, it has a bug as what I commented. If we decide to resign its layer, we can use another CL.
https://codereview.chromium.org/2573183002/diff/540001/base/i18n/time_formatt... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2573183002/diff/540001/base/i18n/time_formatt... base/i18n/time_formatting.h:84: // TODO (chengx) fix function output when width = DURATION_WIDTH_NUMERIC On 2016/12/21 20:45:37, dcheng wrote: > Nit: TODO(chengx): fix function output... > > is the proper formatting. Done. https://codereview.chromium.org/2573183002/diff/540001/base/i18n/time_formatt... base/i18n/time_formatting.h:85: // (http://crbug.com/675791) On 2016/12/21 02:13:40, brucedawson wrote: > Maybe skip these comments. I had a quick look on the fix, which turns out to be not that easy. Since it will take some time for the fix, I will leave the comments here to avoid further misuse on these functions. https://codereview.chromium.org/2573183002/diff/540001/base/i18n/time_formatt... base/i18n/time_formatting.h:99: TimeDurationFormatWithSeconds(const TimeDelta& time, On 2016/12/21 20:45:37, dcheng wrote: > Note that base::Time, base::TimeDelta, base::TimeTicks are designed to be passed > by value. We should do that here. Alright, I changed to pass-by-value for the two TimeDuration functions. For other functions, I will keep them for now or I will use another CL. https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/shared_sampler.h (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler.h:55: const OnCpuTimeCallback& on_cpu_time); On 2016/12/21 19:17:52, stanisc wrote: > If we add any more callbacks we should consider replacing all this with a struct > and pass a struct instead. > > Actually there is already a struct 'Callbacks' defined in .cc file. May be we > should expose it here. If I use 'Callbacks' for RegisterCallbacks() in shared_sampler_win.cc, I need to use it for RegisterCallbacks() in shared_sampler_posix.cc as well. However, 'Callbacks' is only defined in OS_WIN as in the header file. Can I safely remove if if_def(OS_WIN) in the header file? https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler.h:92: uint64_t start_time; // in unit of 100 ns On 2016/12/21 02:13:40, brucedawson wrote: > Should probably be "In units of 100 ns" (capitalization and pluralize 'units') > > Or "Units of 100 ns" Done. https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler_win.cc:499: process_data.start_time = (uint64_t)pi->CreateTime; On 2016/12/21 02:13:40, brucedawson wrote: > Should remove those casts - ULONGLONG and uint64_t are the same, and C style > casts aren't allowed. Done. https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler_win.cc:501: process_data.cpu_time = (uint64_t)(pi->KernelTime + pi->UserTime); On 2016/12/21 02:13:40, brucedawson wrote: > Ditto. Done. https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler_win.cc:611: // Task manager will use this to display 'N/A'. On 2016/12/21 19:17:52, stanisc wrote: > Could you replace 'N/A' with '-'? Done. https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:148: expected_on_bg_done_flags_ = refresh_flags & kBackgroundRefreshTypesMask; On 2016/12/21 18:34:34, afakhry wrote: > Now that we have calculations done only by the |shared_sampler_| and not by the > |worker_thread_sampler_|. This means for non-Windows platforms > OnBackgroundRefreshTypeFinished() will never be called for the start and CPU > times. If someone enables these refresh types by mistake, observers will stop > receiving OnTasksRefreshedWithBackgroundCalculations() signals. > > Here's a suggestion: > We can add TaskGroupSampler::GetSupportedFlags(). Then we'll have: > int64_t expected_shared_flags = > refresh_flags & shared_sampler_->GetSupportedFlags(); > int64_t expected_non_shared_flags = > refresh_flags & worker_thread_sampler_->GetSupportedFlags(); > > expected_on_bg_done_flags_ = > expected_shared_flags | expected_non_shared_flags; Thanks for the suggestion. I think adding an '#if defined(OS_WIN)'for kBackgroundRefreshTypesMask as in line 28 should also work. If using GetSupportedFlags(), we may need to delete kBackgroundRefreshTypesMask as it is no longer useful. Correct me please if I am wrong. https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.h (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.h:153: uint64_t cpu_time_; On 2016/12/21 18:34:34, afakhry wrote: > I think the units need to be documented here as well, and mention they're only > calculated on Windows at the moment. Done. https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager_interface.h (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_interface.h:80: virtual base::TimeDelta GetCpuTime(TaskId task_id) const = 0; On 2016/12/21 18:34:34, afakhry wrote: > Add a comment that these are only implemented for Windows. Done. https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_table_model.cc:138: base::DURATION_WIDTH_NARROW); On 2016/12/21 19:17:52, stanisc wrote: > Why not DURATION_WIDTH_NUMERIC? There is a bug here with DURATION_WIDTH_NUMERIC. It applies the mod operation on hour value by 24, so the hour can never be larger than 24. I have filed a bug (id = 675791) on this. I have had a look and it turns out the fix is fairly complicated. So I would not the NUMERIC format for now.
The CQ bit was checked by chengx@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.
base LGTM with comment addressed https://codereview.chromium.org/2573183002/diff/580001/base/i18n/time_formatt... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2573183002/diff/580001/base/i18n/time_formatt... base/i18n/time_formatting.h:84: // TODO (chengx): fix function output when width = DURATION_WIDTH_NUMERIC Nit: no space between TODO and (
https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2573183002/diff/540001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:148: expected_on_bg_done_flags_ = refresh_flags & kBackgroundRefreshTypesMask; On 2016/12/21 22:05:54, chengx wrote: > On 2016/12/21 18:34:34, afakhry wrote: > > Now that we have calculations done only by the |shared_sampler_| and not by > the > > |worker_thread_sampler_|. This means for non-Windows platforms > > OnBackgroundRefreshTypeFinished() will never be called for the start and CPU > > times. If someone enables these refresh types by mistake, observers will stop > > receiving OnTasksRefreshedWithBackgroundCalculations() signals. > > > > Here's a suggestion: > > We can add TaskGroupSampler::GetSupportedFlags(). Then we'll have: > > int64_t expected_shared_flags = > > refresh_flags & shared_sampler_->GetSupportedFlags(); > > int64_t expected_non_shared_flags = > > refresh_flags & worker_thread_sampler_->GetSupportedFlags(); > > > > expected_on_bg_done_flags_ = > > expected_shared_flags | expected_non_shared_flags; > > Thanks for the suggestion. I think adding an '#if defined(OS_WIN)'for > kBackgroundRefreshTypesMask as in line 28 should also work. If using > GetSupportedFlags(), we may need to delete kBackgroundRefreshTypesMask as it is > no longer useful. Correct me please if I am wrong. Yes, correct. If you go with my suggestion, we'll have to remove kBackgroundRefreshTypesMask. The only problem with kBackgroundRefreshTypesMask is that we have to keep it in sync with what shared and the non-shared samplers offer on various platforms which can get hairy and makes it easy to introduce bugs (as what was about to happen here). So the advantage of the suggestion is that we get to keep that knowledge in the samplers themselves and query them accordingly. That said, I can live with kBackgroundRefreshTypesMask for now. :) https://codereview.chromium.org/2573183002/diff/580001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): https://codereview.chromium.org/2573183002/diff/580001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:103: return base::Time::FromFileTime(ft); I'm wondering why we don't do this calculation in the shared_sampler and make it return the ready-made base::Time and base::TimeDelta values for both the start and CPU times respectively to the TaskGroup. Let's keep internal things internal. How about that?
Patchset #8 (id:600001) has been deleted
Patchset #9 (id:640001) has been deleted
Patchset #8 (id:620001) has been deleted
Patchset #9 (id:680001) has been deleted
Patchset #8 (id:660001) has been deleted
The CQ bit was checked by chengx@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.
afakhry@ and ncarter@, PTAL~ https://codereview.chromium.org/2573183002/diff/580001/base/i18n/time_formatt... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2573183002/diff/580001/base/i18n/time_formatt... base/i18n/time_formatting.h:84: // TODO (chengx): fix function output when width = DURATION_WIDTH_NUMERIC On 2016/12/21 23:28:57, dcheng wrote: > Nit: no space between TODO and ( Done. https://codereview.chromium.org/2573183002/diff/580001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): https://codereview.chromium.org/2573183002/diff/580001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:103: return base::Time::FromFileTime(ft); On 2016/12/22 00:28:14, afakhry (OOO Dec 22 - Jan 2) wrote: > I'm wondering why we don't do this calculation in the shared_sampler and make it > return the ready-made base::Time and base::TimeDelta values for both the start > and CPU times respectively to the TaskGroup. > > Let's keep internal things internal. How about that? Thanks for the suggestion! I have moved the ticks-Time and ticks-TimeDelta conversions into shared_sampler_win.cc.
Patchset #9 (id:720001) has been deleted
Kind reminder. afakhry@ and ncarter@, PTAL~ Thanks!
afakhry@ and ncarter@, PTAL~ Thanks!
lgtm with nits. https://codereview.chromium.org/2573183002/diff/700001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2573183002/diff/700001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler_win.cc:446: namespace { Nit: Usually we have a single anonymous namespace at the top of the file. https://codereview.chromium.org/2573183002/diff/700001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler_win.cc:457: } Nit: } // namespace https://codereview.chromium.org/2573183002/diff/700001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): https://codereview.chromium.org/2573183002/diff/700001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:99: #else Nit: Can you please add a NOTIMPLEMENTED(); here? https://codereview.chromium.org/2573183002/diff/700001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:107: #else Here too?
Patchset #9 (id:740001) has been deleted
The CQ bit was checked by chengx@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 chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stanisc@chromium.org, brucedawson@chromium.org, dcheng@chromium.org, afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2573183002/#ps780001 (title: "Fix nits and conflicts.")
afakhry@, I have addressed the nits in the new patch. Thanks! https://codereview.chromium.org/2573183002/diff/700001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/shared_sampler_win.cc (right): https://codereview.chromium.org/2573183002/diff/700001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler_win.cc:446: namespace { On 2017/01/09 21:03:42, afakhry wrote: > Nit: Usually we have a single anonymous namespace at the top of the file. I have moved these two functions to the existing anonymous namespace above. https://codereview.chromium.org/2573183002/diff/700001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/shared_sampler_win.cc:457: } On 2017/01/09 21:03:42, afakhry wrote: > Nit: } // namespace Done. https://codereview.chromium.org/2573183002/diff/700001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): https://codereview.chromium.org/2573183002/diff/700001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:99: #else On 2017/01/09 21:03:43, afakhry wrote: > Nit: Can you please add a NOTIMPLEMENTED(); here? Done. https://codereview.chromium.org/2573183002/diff/700001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:107: #else On 2017/01/09 21:03:42, afakhry wrote: > Here too? Done.
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": 780001, "attempt_start_ts": 1484008727506450,
"parent_rev": "46b9f6aaaf5851014c2474365734e71ffe9367a0", "commit_rev":
"848410dc4e71fa3ce9727478ab013a8948c4dfc6"}
Message was sent while issue was closed.
Description was changed from ========== Add process start time and CPU time columns to task manager Both start time and CPU time are obtained using shared_sampler_win.cc. By default, they won't show up. BUG=670921 ========== to ========== Add process start time and CPU time columns to task manager Both start time and CPU time are obtained using shared_sampler_win.cc. By default, they won't show up. BUG=670921 Review-Url: https://codereview.chromium.org/2573183002 Cr-Commit-Position: refs/heads/master@{#442449} Committed: https://chromium.googlesource.com/chromium/src/+/848410dc4e71fa3ce9727478ab01... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:780001) as https://chromium.googlesource.com/chromium/src/+/848410dc4e71fa3ce9727478ab01... |
