Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2362743002:
Add TaskScheduler.NumTasksBetweenWaits.[pool] histogram. (Closed)
DescriptionAdd TaskScheduler.NumTasksBetweenWaits.[pool] histogram.
This histogram gives the number of tasks executed by a
SchedulerWorker between two waits on its WaitableEvent.
BUG=553459
Committed: https://crrev.com/628580f4cb1d4bf9c834ee9fd203b9c69def1417
Cr-Commit-Position: refs/heads/master@{#421324}
Patch Set 1 #Patch Set 2 : self-review #Patch Set 3 : self-review #Patch Set 4 : self-review #
Total comments: 2
Patch Set 5 : CR gab #7 #Patch Set 6 : self-review #
Total comments: 6
Patch Set 7 : CR gab/robliao #7-10 #
Total comments: 10
Patch Set 8 : CR robliao #14 #Patch Set 9 : CR robliao #18 #Patch Set 10 : fix build error #Patch Set 11 : fix test error #
Messages
Total messages: 39 (14 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
What's the relationship with 310211? Also, I don't see this histogram and its justification in https://docs.google.com/document/d/12eCfWDWf0TMxXU2oEJjks1mfJApx0hYnfY0so63Mz... ? And lastly, without looking at the code, I'm a bit confused by the definition in CL desc, "between when two periods"?
Description was changed from ========== Add TaskScheduler.NumTasksBetweenWaits.[pool] histogram. This histogram gives the number of tasks executed by a SchedulerWorker between when two periods during which it was waiting for a WakeUp(). A SchedulerWorker may or may not be detached when it waits for a WakeUp(). BUG=310211 ========== to ========== Add TaskScheduler.NumTasksBetweenWaits.[pool] histogram. This histogram gives the number of tasks executed by a SchedulerWorker between when two waits on its WaitableEvent. BUG=553459 ==========
Description was changed from ========== Add TaskScheduler.NumTasksBetweenWaits.[pool] histogram. This histogram gives the number of tasks executed by a SchedulerWorker between when two waits on its WaitableEvent. BUG=553459 ========== to ========== Add TaskScheduler.NumTasksBetweenWaits.[pool] histogram. This histogram gives the number of tasks executed by a SchedulerWorker between two waits on its WaitableEvent. BUG=553459 ==========
On 2016/09/22 18:00:42, gab wrote: > What's the relationship with 310211? > > Also, I don't see this histogram and its justification in > https://docs.google.com/document/d/12eCfWDWf0TMxXU2oEJjks1mfJApx0hYnfY0so63Mz... > ? > > And lastly, without looking at the code, I'm a bit confused by the definition in > CL desc, "between when two periods"? Updated the description + bug link. Added the histogram to the doc. IMO, this histogram is better than sampling the number of times that threads become idle during a 30-second time interval.
High-level lgtm, deferring to robliao for implementation details. https://codereview.chromium.org/2362743002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2362743002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62593: + WaitableEvent. Add to the description that the goal is to maximize this without affecting TaskScheduler.TaskLatency.*
robliao@: PTAL https://codereview.chromium.org/2362743002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2362743002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62593: + WaitableEvent. On 2016/09/22 18:43:45, gab wrote: > Add to the description that the goal is to maximize this without affecting > TaskScheduler.TaskLatency.* Done.
On 2016/09/22 19:24:47, fdoray wrote: > robliao@: PTAL > > https://codereview.chromium.org/2362743002/diff/60001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2362743002/diff/60001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:62593: + WaitableEvent. > On 2016/09/22 18:43:45, gab wrote: > > Add to the description that the goal is to maximize this without affecting > > TaskScheduler.TaskLatency.* > > Done. Hmm.. I'm not sure that not affecting TaskScheduler.TaskLatency.* is a goal. Not waking up a thread to run a task may increase its latency. The goal is not to affect startup time, page load time, etc.
On 2016/09/22 19:29:05, fdoray wrote: > On 2016/09/22 19:24:47, fdoray wrote: > > robliao@: PTAL > > > > > https://codereview.chromium.org/2362743002/diff/60001/tools/metrics/histogram... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/2362743002/diff/60001/tools/metrics/histogram... > > tools/metrics/histograms/histograms.xml:62593: + WaitableEvent. > > On 2016/09/22 18:43:45, gab wrote: > > > Add to the description that the goal is to maximize this without affecting > > > TaskScheduler.TaskLatency.* > > > > Done. > > Hmm.. I'm not sure that not affecting TaskScheduler.TaskLatency.* is a goal. Not > waking up a thread to run a task may increase its latency. The goal is not to > affect startup time, page load time, etc. Fair enough, PS6 wording lgtm
lgtm % comments. How hard would it be to verify that we're sending these histograms in the unit tests? https://codereview.chromium.org/2362743002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2362743002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl.cc:522: // |did_detach_since_last_get_work_| is set to true and the next call to I would say here that did_detach_since_last_get_work_ will be set to true after a thread has detached and the thread is woken up again in OnMainEntry. https://codereview.chromium.org/2362743002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl.cc:583: outer_->RemoveFromIdleWorkersStack(worker); Nit: The ordering here isn't sensitive, right? Let's group the local state update together and do the removal last (or vice versa, as long as its grouped). idle_start_time_ = ... last_get_work_returned_nullptr_ = ... https://codereview.chromium.org/2362743002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl.cc:673: // Mimics the UMA_HISTOGRAM_COUNTS_10000 macro. Worth mentioning why we chose the 10000 macro to count number of tasks executed between waits.
fdoray@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow@: PTAL robliao@: Done. I added tests. https://codereview.chromium.org/2362743002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2362743002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl.cc:522: // |did_detach_since_last_get_work_| is set to true and the next call to On 2016/09/23 00:55:30, robliao wrote: > I would say here that did_detach_since_last_get_work_ will be set to true after > a thread has detached and the thread is woken up again in OnMainEntry. Done. https://codereview.chromium.org/2362743002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl.cc:583: outer_->RemoveFromIdleWorkersStack(worker); On 2016/09/23 00:55:30, robliao wrote: > Nit: The ordering here isn't sensitive, right? Let's group the local state > update together and do the removal last (or vice versa, as long as its grouped). > > idle_start_time_ = ... > last_get_work_returned_nullptr_ = ... Local state update is now grouped together, and the order is consistent with the "return nullptr" above. https://codereview.chromium.org/2362743002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl.cc:673: // Mimics the UMA_HISTOGRAM_COUNTS_10000 macro. On 2016/09/23 00:55:30, robliao wrote: > Worth mentioning why we chose the 10000 macro to count number of tasks executed > between waits. Done.
Looking generally good. Nice tests! https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:604: Histogram::FactoryGet( Abstract this as a static call on the SchedulerWorkerPoolImpl. https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:617: static void IncrementCountAndWaitForEvent() {} Stray function? https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:655: static void SignalAndWaitEvent(WaitableEvent* signal_event, Wrap in anonymous namespace, remove static.
rkaplow@: PTAL https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:604: Histogram::FactoryGet( On 2016/09/23 19:44:19, robliao wrote: > Abstract this as a static call on the SchedulerWorkerPoolImpl. FRIEND_TEST_ALL_PREFIXES in SchedulerWorkerPoolImpl instead? That way I don't have to add a static method that wouldn't have existed otherwise. https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:617: static void IncrementCountAndWaitForEvent() {} On 2016/09/23 19:44:19, robliao wrote: > Stray function? Done. https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:655: static void SignalAndWaitEvent(WaitableEvent* signal_event, On 2016/09/23 19:44:19, robliao wrote: > Wrap in anonymous namespace, remove static. Done.
https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:604: Histogram::FactoryGet( On 2016/09/26 12:22:05, fdoray wrote: > On 2016/09/23 19:44:19, robliao wrote: > > Abstract this as a static call on the SchedulerWorkerPoolImpl. > > FRIEND_TEST_ALL_PREFIXES in SchedulerWorkerPoolImpl instead? That way I don't > have to add a static method that wouldn't have existed otherwise. I'd say go for the static method. It's relatively cheap (if not free via optimizations), and pretty clean. This way the test doesn't have access to all the internals for SchedulerWorkerPoolImpl.
Question for robliao@ https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:604: Histogram::FactoryGet( On 2016/09/26 17:19:01, robliao wrote: > On 2016/09/26 12:22:05, fdoray wrote: > > On 2016/09/23 19:44:19, robliao wrote: > > > Abstract this as a static call on the SchedulerWorkerPoolImpl. > > > > FRIEND_TEST_ALL_PREFIXES in SchedulerWorkerPoolImpl instead? That way I don't > > have to add a static method that wouldn't have existed otherwise. > > I'd say go for the static method. It's relatively cheap (if not free via > optimizations), and pretty clean. This way the test doesn't have access to all > the internals for SchedulerWorkerPoolImpl. WDYT of a public accessor for |num_tasks_between_waits_histogram_|? If I add a static method, I'm afraid that it won't be clear that the cached |num_tasks_between_waits_histogram_| must be used instead of calling the static method every time that we record to the histogram.
https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:604: Histogram::FactoryGet( On 2016/09/26 19:14:36, fdoray wrote: > On 2016/09/26 17:19:01, robliao wrote: > > On 2016/09/26 12:22:05, fdoray wrote: > > > On 2016/09/23 19:44:19, robliao wrote: > > > > Abstract this as a static call on the SchedulerWorkerPoolImpl. > > > > > > FRIEND_TEST_ALL_PREFIXES in SchedulerWorkerPoolImpl instead? That way I > don't > > > have to add a static method that wouldn't have existed otherwise. > > > > I'd say go for the static method. It's relatively cheap (if not free via > > optimizations), and pretty clean. This way the test doesn't have access to all > > the internals for SchedulerWorkerPoolImpl. > > WDYT of a public accessor for |num_tasks_between_waits_histogram_|? If I add a > static method, I'm afraid that it won't be clear that the cached > |num_tasks_between_waits_histogram_| must be used instead of calling the static > method every time that we record to the histogram. A public accessor sounds fine too. Accomplishes a similar result.
PTAnL https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2362743002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:604: Histogram::FactoryGet( On 2016/09/26 19:26:38, robliao wrote: > On 2016/09/26 19:14:36, fdoray wrote: > > On 2016/09/26 17:19:01, robliao wrote: > > > On 2016/09/26 12:22:05, fdoray wrote: > > > > On 2016/09/23 19:44:19, robliao wrote: > > > > > Abstract this as a static call on the SchedulerWorkerPoolImpl. > > > > > > > > FRIEND_TEST_ALL_PREFIXES in SchedulerWorkerPoolImpl instead? That way I > > don't > > > > have to add a static method that wouldn't have existed otherwise. > > > > > > I'd say go for the static method. It's relatively cheap (if not free via > > > optimizations), and pretty clean. This way the test doesn't have access to > all > > > the internals for SchedulerWorkerPoolImpl. > > > > WDYT of a public accessor for |num_tasks_between_waits_histogram_|? If I add a > > static method, I'm afraid that it won't be clear that the cached > > |num_tasks_between_waits_histogram_| must be used instead of calling the > static > > method every time that we record to the histogram. > > A public accessor sounds fine too. Accomplishes a similar result. Done.
lgtm. Thanks!
rkaplow@: PTAL
lgtm
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2362743002/#ps160001 (title: "CR robliao #18")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2362743002/#ps180001 (title: "fix build error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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 fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2362743002/#ps200001 (title: "fix test error")
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 ========== Add TaskScheduler.NumTasksBetweenWaits.[pool] histogram. This histogram gives the number of tasks executed by a SchedulerWorker between two waits on its WaitableEvent. BUG=553459 ========== to ========== Add TaskScheduler.NumTasksBetweenWaits.[pool] histogram. This histogram gives the number of tasks executed by a SchedulerWorker between two waits on its WaitableEvent. BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add TaskScheduler.NumTasksBetweenWaits.[pool] histogram. This histogram gives the number of tasks executed by a SchedulerWorker between two waits on its WaitableEvent. BUG=553459 ========== to ========== Add TaskScheduler.NumTasksBetweenWaits.[pool] histogram. This histogram gives the number of tasks executed by a SchedulerWorker between two waits on its WaitableEvent. BUG=553459 Committed: https://crrev.com/628580f4cb1d4bf9c834ee9fd203b9c69def1417 Cr-Commit-Position: refs/heads/master@{#421324} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/628580f4cb1d4bf9c834ee9fd203b9c69def1417 Cr-Commit-Position: refs/heads/master@{#421324} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
