Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1851403003:
TaskScheduler [8] SEQUENCED TaskRunners in SchedulerThreadPool. (Closed)
DescriptionTaskScheduler [8] SEQUENCED TaskRunners in SchedulerThreadPool.
Tasks posted through a SEQUENCED TaskRunner run one at a time in posting
order.
This change is a subset of https://codereview.chromium.org/1698183005/
BUG=553459
Committed: https://crrev.com/28cb618e5ebc1ed5fe87c6e25be459f54bddca42
Cr-Commit-Position: refs/heads/master@{#387746}
Patch Set 1 #
Total comments: 6
Patch Set 2 : CR robliao #4 #Patch Set 3 : self review #Patch Set 4 : rebase #Patch Set 5 : self review #Patch Set 6 : self review #
Total comments: 8
Patch Set 7 : CR gab/robliao #7-8 #Patch Set 8 : self review #Patch Set 9 : rebase #Patch Set 10 : self review #Patch Set 11 : rebase #
Total comments: 3
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 22 (8 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
robliao@: Can you review this CL? Thanks.
Description was changed from ========== Task Scheduler [9] SEQUENCED TaskRunners in SchedulerThreadPool. Tasks posted through a SEQUENCED TaskRunner run one at a time in posting order. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [9] SEQUENCED TaskRunners in SchedulerThreadPool. Tasks posted through a SEQUENCED TaskRunner run one at a time in posting order. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
https://codereview.chromium.org/1851403003/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1851403003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_thread_pool_unittest.cc:77: TaskFactory(scoped_refptr<TaskRunner> task_runner, I wonder if there's a way we can extract this out of a task runner. I currently don't see an easy way to do this at the moment. https://codereview.chromium.org/1851403003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_thread_pool_unittest.cc:201: thread_pool_.get(), ExecutionMode::PARALLEL, false, false))); Should PARALLEL be parameterized? All of the tests below do not change this parameter. https://codereview.chromium.org/1851403003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_thread_pool_unittest.cc:273: factories.push_back(make_scoped_ptr(new TaskFactory( Should this really be a part of |factories| or does it make more sense for this to be outside of |factories|?
robliao@: Can you take another look? https://codereview.chromium.org/1851403003/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1851403003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_thread_pool_unittest.cc:77: TaskFactory(scoped_refptr<TaskRunner> task_runner, On 2016/04/04 18:45:50, robliao wrote: > I wonder if there's a way we can extract this out of a task runner. I currently > don't see an easy way to do this at the moment. I changed the code to avoid relying on the caller to provide matching task runner / execution mode. https://codereview.chromium.org/1851403003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_thread_pool_unittest.cc:201: thread_pool_.get(), ExecutionMode::PARALLEL, false, false))); On 2016/04/04 18:45:50, robliao wrote: > Should PARALLEL be parameterized? All of the tests below do not change this > parameter. Done. https://codereview.chromium.org/1851403003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_thread_pool_unittest.cc:273: factories.push_back(make_scoped_ptr(new TaskFactory( On 2016/04/04 18:45:50, robliao wrote: > Should this really be a part of |factories| or does it make more sense for this > to be outside of |factories|? Done.
Description was changed from ========== TaskScheduler [9] SEQUENCED TaskRunners in SchedulerThreadPool. Tasks posted through a SEQUENCED TaskRunner run one at a time in posting order. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [8] SEQUENCED TaskRunners in SchedulerThreadPool. Tasks posted through a SEQUENCED TaskRunner run one at a time in posting order. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
lgtm w/ nits https://codereview.chromium.org/1851403003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1851403003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:89: task_runner_->PostTask( Doesn't hurt to keep the EXPECT_TRUE I guess (although we know the impl never returns false it's still okay to test it?) https://codereview.chromium.org/1851403003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:297: // Verify that it is possible to have |kNumThreadsInThreadPool| tasks running "tasks running" => "tasks/sequences running" ? to highlight that it's also testing that independent sequences can also be scheduled when other sequences are blocked?
lgtm https://codereview.chromium.org/1851403003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1851403003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:89: task_runner_->PostTask( On 2016/04/08 16:52:25, gab wrote: > Doesn't hurt to keep the EXPECT_TRUE I guess (although we know the impl never > returns false it's still okay to test it?) If we're operating on the premise that the return value of PostTask doesn't matter, it would be reasonable to leave this as is and not encourage others to check PostTask results. Failures should surface themselves as test failures. https://codereview.chromium.org/1851403003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:271: // the execution mode is SEQUENCED. Worth mentioning that we do this to avoid sequence checking presumably? https://codereview.chromium.org/1851403003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:297: // Verify that it is possible to have |kNumThreadsInThreadPool| tasks running On 2016/04/08 16:52:25, gab wrote: > "tasks running" => "tasks/sequences running" ? to highlight that it's also > testing that independent sequences can also be scheduled when other sequences > are blocked? Hrm... this is an interesting point. We are running tasks, and every task is associated with a sequence, so that sequence is also running.
https://codereview.chromium.org/1851403003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1851403003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:89: task_runner_->PostTask( On 2016/04/08 17:11:11, robliao wrote: > On 2016/04/08 16:52:25, gab wrote: > > Doesn't hurt to keep the EXPECT_TRUE I guess (although we know the impl never > > returns false it's still okay to test it?) > > If we're operating on the premise that the return value of PostTask doesn't > matter, it would be reasonable to leave this as is and not encourage others to > check PostTask results. > > Failures should surface themselves as test failures. Put back the EXPECT_TRUE. The impl returns false when a CONTINUE/SKIP_ON_SHUTDOWN task is posted during shutdown. https://codereview.chromium.org/1851403003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:271: // the execution mode is SEQUENCED. On 2016/04/08 17:11:12, robliao wrote: > Worth mentioning that we do this to avoid sequence checking presumably? Done. https://codereview.chromium.org/1851403003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:297: // Verify that it is possible to have |kNumThreadsInThreadPool| tasks running On 2016/04/08 17:11:12, robliao wrote: > On 2016/04/08 16:52:25, gab wrote: > > "tasks running" => "tasks/sequences running" ? to highlight that it's also > > testing that independent sequences can also be scheduled when other sequences > > are blocked? > > Hrm... this is an interesting point. > We are running tasks, and every task is associated with a sequence, so that > sequence is also running. Done.
fdoray@chromium.org changed reviewers: + danakj@chromium.org
danakj@: Can you review this CL once you have reviewed the SchedulerThreadPool CL? Thanks.
danakj@: I rebased this CL. Can you review it?
LGTM https://codereview.chromium.org/1851403003/diff/200001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1851403003/diff/200001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:319: event.Signal(); Could we maybe verify that all the tasks actually started running before releasing/ending them?
Thanks for the reviews! https://codereview.chromium.org/1851403003/diff/200001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1851403003/diff/200001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:319: event.Signal(); On 2016/04/15 21:14:34, danakj wrote: > Could we maybe verify that all the tasks actually started running before > releasing/ending them? This is done by line 315.
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 Link to the patchset: https://codereview.chromium.org/1851403003/#ps200001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1851403003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1851403003/200001
https://codereview.chromium.org/1851403003/diff/200001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1851403003/diff/200001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:319: event.Signal(); On 2016/04/15 21:49:41, fdoray wrote: > On 2016/04/15 21:14:34, danakj wrote: > > Could we maybe verify that all the tasks actually started running before > > releasing/ending them? > > This is done by line 315. Ah, word ok. I was expecting to see kNumThreadsInThreadPool in some check.
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [8] SEQUENCED TaskRunners in SchedulerThreadPool. Tasks posted through a SEQUENCED TaskRunner run one at a time in posting order. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [8] SEQUENCED TaskRunners in SchedulerThreadPool. Tasks posted through a SEQUENCED TaskRunner run one at a time in posting order. This change is a subset of https://codereview.chromium.org/1698183005/ 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 ========== TaskScheduler [8] SEQUENCED TaskRunners in SchedulerThreadPool. Tasks posted through a SEQUENCED TaskRunner run one at a time in posting order. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [8] SEQUENCED TaskRunners in SchedulerThreadPool. Tasks posted through a SEQUENCED TaskRunner run one at a time in posting order. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 Committed: https://crrev.com/28cb618e5ebc1ed5fe87c6e25be459f54bddca42 Cr-Commit-Position: refs/heads/master@{#387746} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/28cb618e5ebc1ed5fe87c6e25be459f54bddca42 Cr-Commit-Position: refs/heads/master@{#387746} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
