Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2412343003:
Add TaskScheduler.NumTasksBeforeDetach.[worker pool name] histogram. (Closed)
DescriptionAdd TaskScheduler.NumTasksBeforeDetach.[worker pool name] histogram.
Number of tasks executed by a SchedulerWorker before it detached. Recorded
when the SchedulerWorker is woken up after detaching. A low value indicates
that SchedulerWorkers are detached too often.
BUG=553459
Committed: https://crrev.com/ac8f31454430b1f94a18e311f1cbc42d095d3b53
Cr-Commit-Position: refs/heads/master@{#425229}
Patch Set 1 #Patch Set 2 : self-review #
Total comments: 6
Patch Set 3 : CR robliao #3 #
Total comments: 25
Depends on Patchset: Messages
Total messages: 18 (6 generated)
fdoray@chromium.org changed reviewers: + robliao@chromium.org
PTAL
lgtm % nits. https://codereview.chromium.org/2412343003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.h (right): https://codereview.chromium.org/2412343003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:97: const HistogramBase* num_tasks_before_detach_histogram_for_testing() const { Feel free to remove the _for_testing portion of this getting. I'll be referencing them in the upcoming taskscheduler stats page change. https://codereview.chromium.org/2412343003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2412343003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:597: if (worker_pool_) Mention that worker pool initialization is handled by the specific test instead of the test case. Can be done later: It would be nice if we could move initialization to test case SetUp. https://codereview.chromium.org/2412343003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2412343003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64335: + that SchedulerWorkers are detached too often. Nit: Another conclusion could be the rate of work is low. Let's remove the last sentence.
fdoray@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow@: Please review histogram changes. https://codereview.chromium.org/2412343003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.h (right): https://codereview.chromium.org/2412343003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:97: const HistogramBase* num_tasks_before_detach_histogram_for_testing() const { On 2016/10/13 18:04:04, robliao wrote: > Feel free to remove the _for_testing portion of this getting. I'll be > referencing them in the upcoming taskscheduler stats page change. Done. https://codereview.chromium.org/2412343003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2412343003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:597: if (worker_pool_) On 2016/10/13 18:04:04, robliao wrote: > Mention that worker pool initialization is handled by the specific test instead > of the test case. Done > Can be done later: It would be nice if we could move initialization to test case > SetUp. The problem with that is that now all test cases initialize the worker pool with the same params. I don't know how to have code in SetUp() that behaves differently depending on the test case. https://codereview.chromium.org/2412343003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2412343003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64335: + that SchedulerWorkers are detached too often. On 2016/10/13 18:04:04, robliao wrote: > Nit: Another conclusion could be the rate of work is low. Let's remove the last > sentence. Done.
lgtm
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 Link to the patchset: https://codereview.chromium.org/2412343003/#ps40001 (title: "CR robliao #3")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add TaskScheduler.NumTasksBeforeDetach.[worker pool name] histogram. Number of tasks executed by a SchedulerWorker before it detached. Recorded when the SchedulerWorker is woken up after detaching. A low value indicates that SchedulerWorkers are detached too often. BUG=553459 ========== to ========== Add TaskScheduler.NumTasksBeforeDetach.[worker pool name] histogram. Number of tasks executed by a SchedulerWorker before it detached. Recorded when the SchedulerWorker is woken up after detaching. A low value indicates that SchedulerWorkers are detached too often. BUG=553459 Committed: https://crrev.com/ac8f31454430b1f94a18e311f1cbc42d095d3b53 Cr-Commit-Position: refs/heads/master@{#425229} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ac8f31454430b1f94a18e311f1cbc42d095d3b53 Cr-Commit-Position: refs/heads/master@{#425229}
Message was sent while issue was closed.
gab@chromium.org changed reviewers: + gab@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:498: outer_->num_tasks_before_detach_histogram_->Add( Recording here will only record if the thread is recreated. This seems weird to me. Why not record on detach? https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:690: // the exact number of tasks that ran. While I agree that once it runs more than a 1000 tasks we can call that a "good run for the money" and not care how many exactly (i.e. it was worth it to wake it up). I don't think it's true that "a SW is expected to run between zero and a few hundreds of tasks before detaching". https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:594: void SetUp() override {} drive-by nit: No-op override? https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:599: worker_pool_->JoinForTesting(); This code belongs in TaskSchedulerWorkerPoolImplTest::TearDown() IMO as it owns |worker_pool_|. Also: overriding TearDown of another fixture should always be followed by calling the parent class' TearDown method (which doesn't make sense here hence why there just shouldn't be an override) https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:606: private: drive-by nit: double private https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:725: TaskTraits(), ExecutionMode::SINGLE_THREADED); This test assumes both of these aren't assigned the same thread. Can you add something that documents and verifies that (so it's easier to understand what's going on if we one day change the thread assignment algo and this test starts failing -- or worst flaking). https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:738: task_runner = nullptr; nit: I prefer |task_runner.Release()| here to explicitly document that this is releasing the ref (just like we use .reset() on unique_ptr's) https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:746: worker_pool_->DisallowWorkerDetachmentForTesting(); Is this required? If we don't post any more tasks, nothing should be awaken and thus nothing should re-detach, right? https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:749: other_task_runner = nullptr; .Release()
Message was sent while issue was closed.
https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:498: outer_->num_tasks_before_detach_histogram_->Add( On 2016/10/17 18:24:00, gab (OOO) wrote: > Recording here will only record if the thread is recreated. This seems weird to > me. Why not record on detach? The SchedulerWorker is free to ignore the detach request (e.g. race condition where a wakeup is pending while the scheduler worker gets ready to detach). https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:594: void SetUp() override {} On 2016/10/17 18:24:00, gab (OOO) wrote: > drive-by nit: No-op override? This suppresses the common worker_pool_ init and allow the test case itself to init it. https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:746: worker_pool_->DisallowWorkerDetachmentForTesting(); On 2016/10/17 18:24:00, gab (OOO) wrote: > Is this required? If we don't post any more tasks, nothing should be awaken and > thus nothing should re-detach, right? It's a nice to have for test cleanliness for the subsequent JoinForTesting.
Message was sent while issue was closed.
https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:498: outer_->num_tasks_before_detach_histogram_->Add( On 2016/10/17 23:05:39, robliao wrote: > On 2016/10/17 18:24:00, gab (OOO) wrote: > > Recording here will only record if the thread is recreated. This seems weird > to > > me. Why not record on detach? > > The SchedulerWorker is free to ignore the detach request (e.g. race condition > where a wakeup is pending while the scheduler worker gets ready to detach). Right but we could still add SchedulerWorkerDelegate::OnDetach() and call it at the end of a successful SchedulerWorker::Detach(). The ideal detach is one where we never re-attach and in this case the metric will ignore those and thus be incorrectly skewed IMO. https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:594: void SetUp() override {} On 2016/10/17 23:05:39, robliao wrote: > On 2016/10/17 18:24:00, gab (OOO) wrote: > > drive-by nit: No-op override? > > This suppresses the common worker_pool_ init and allow the test case itself to > init it. Hmmm okay, then how about a comment stating that this is intentional. https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:746: worker_pool_->DisallowWorkerDetachmentForTesting(); On 2016/10/17 23:05:39, robliao wrote: > On 2016/10/17 18:24:00, gab (OOO) wrote: > > Is this required? If we don't post any more tasks, nothing should be awaken > and > > thus nothing should re-detach, right? > > It's a nice to have for test cleanliness for the subsequent JoinForTesting. IDK, to me any code that should never do anything decreases readability as a reader has to think why it matters that it's there.
Message was sent while issue was closed.
Comments addressed in https://codereview.chromium.org/2430633003/ https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:498: outer_->num_tasks_before_detach_histogram_->Add( On 2016/10/18 14:45:45, gab wrote: > On 2016/10/17 23:05:39, robliao wrote: > > On 2016/10/17 18:24:00, gab (OOO) wrote: > > > Recording here will only record if the thread is recreated. This seems weird > > to > > > me. Why not record on detach? > > > > The SchedulerWorker is free to ignore the detach request (e.g. race condition > > where a wakeup is pending while the scheduler worker gets ready to detach). > > Right but we could still add SchedulerWorkerDelegate::OnDetach() and call it at > the end of a successful SchedulerWorker::Detach(). > > The ideal detach is one where we never re-attach and in this case the metric > will ignore those and thus be incorrectly skewed IMO. Done. https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:690: // the exact number of tasks that ran. On 2016/10/17 18:24:00, gab wrote: > While I agree that once it runs more than a 1000 tasks we can call that a "good > run for the money" and not care how many exactly (i.e. it was worth it to wake > it up). > > I don't think it's true that "a SW is expected to run between zero and a few > hundreds of tasks before detaching". Done. https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:594: void SetUp() override {} On 2016/10/18 14:45:45, gab wrote: > On 2016/10/17 23:05:39, robliao wrote: > > On 2016/10/17 18:24:00, gab (OOO) wrote: > > > drive-by nit: No-op override? > > > > This suppresses the common worker_pool_ init and allow the test case itself to > > init it. > > Hmmm okay, then how about a comment stating that this is intentional. Done. https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:599: worker_pool_->JoinForTesting(); On 2016/10/17 18:24:00, gab wrote: > This code belongs in TaskSchedulerWorkerPoolImplTest::TearDown() IMO as it owns > |worker_pool_|. > > Also: overriding TearDown of another fixture should always be followed by > calling the parent class' TearDown method (which doesn't make sense here hence > why there just shouldn't be an override) Done. https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:606: private: On 2016/10/17 18:24:00, gab wrote: > drive-by nit: double private Done. https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:725: TaskTraits(), ExecutionMode::SINGLE_THREADED); On 2016/10/17 18:24:00, gab wrote: > This test assumes both of these aren't assigned the same thread. Can you add > something that documents and verifies that (so it's easier to understand what's > going on if we one day change the thread assignment algo and this test starts > failing -- or worst flaking). Done. https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:738: task_runner = nullptr; On 2016/10/17 18:24:00, gab wrote: > nit: I prefer |task_runner.Release()| here to explicitly document that this is > releasing the ref (just like we use .reset() on unique_ptr's) Doesn't work. Release() is a private static method https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?l=397 https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:746: worker_pool_->DisallowWorkerDetachmentForTesting(); On 2016/10/18 14:45:46, gab wrote: > On 2016/10/17 23:05:39, robliao wrote: > > On 2016/10/17 18:24:00, gab (OOO) wrote: > > > Is this required? If we don't post any more tasks, nothing should be awaken > > and > > > thus nothing should re-detach, right? > > > > It's a nice to have for test cleanliness for the subsequent JoinForTesting. > > IDK, to me any code that should never do anything decreases readability as a > reader has to think why it matters that it's there. DisallowWorkerDetachmentForTesting() must be called before JoinForTesting() when the detach time is not TimeDelta::Max() https://cs.chromium.org/chromium/src/base/task_scheduler/scheduler_worker_poo... https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:749: other_task_runner = nullptr; On 2016/10/17 18:24:00, gab wrote: > .Release() N/A
Message was sent while issue was closed.
Thanks for follow-up https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2412343003/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:738: task_runner = nullptr; On 2016/10/18 19:41:23, fdoray wrote: > On 2016/10/17 18:24:00, gab wrote: > > nit: I prefer |task_runner.Release()| here to explicitly document that this is > > releasing the ref (just like we use .reset() on unique_ptr's) > > Doesn't work. Release() is a private static method > https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?l=397 Ah interesting, somehow was convinced it was exposed. I understand why Add/Remove refs isn't exposed but thought scoped_refptr<>::Release() would be exposed as it can't introduce miscounting (i.e. it would both release the ref and make that pointer null). Oh well :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
