Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2367843002:
Support SequencedWorkerPool::FlushForTesting() with redirection to TaskScheduler. (Closed)
DescriptionSupport SequencedWorkerPool::FlushForTesting() with redirection to TaskScheduler.
BUG=622400
Committed: https://crrev.com/bca8090714d6ca08679a3bc6b3407e280acd723e
Cr-Commit-Position: refs/heads/master@{#421194}
Patch Set 1 #
Total comments: 6
Patch Set 2 : CR gab #4 #
Total comments: 9
Patch Set 3 : CR danakj #8 #
Total comments: 3
Patch Set 4 : CR danakj #10 #
Messages
Total messages: 25 (9 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
lgmt % comments https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:900: TaskScheduler::GetInstance()->FlushForTesting(); How about keeping this method only for non-redirection and doing the redirection in SequencedWorkerPool::FlushForTesting() instead? https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.h:339: // to TaskScheduler is disabled. s/if redirection to TaskScheduler is disabled/unless redirection to TaskScheduler is enabled/ https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:990: // TaskScheduler::FlushForTesting() does not delete delayed tasks. But they are deleted eventually (i.e. with ~TaskScheduler()), right?. Mention that here, something like: // TaskScheduler deletes unexecuted delayed tasks as part of ~TaskScheduler() instead of TaskScheduler::FlushForTesting().
On 2016/09/23 15:44:12, gab wrote: > lgmt % comments lgtm that is ;-) > > https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... > File base/threading/sequenced_worker_pool.cc (right): > > https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... > base/threading/sequenced_worker_pool.cc:900: > TaskScheduler::GetInstance()->FlushForTesting(); > How about keeping this method only for non-redirection and doing the redirection > in SequencedWorkerPool::FlushForTesting() instead? > > https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... > File base/threading/sequenced_worker_pool.h (right): > > https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... > base/threading/sequenced_worker_pool.h:339: // to TaskScheduler is disabled. > s/if redirection to TaskScheduler is disabled/unless redirection to > TaskScheduler is enabled/ > > https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... > File base/threading/sequenced_worker_pool_unittest.cc (right): > > https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... > base/threading/sequenced_worker_pool_unittest.cc:990: // > TaskScheduler::FlushForTesting() does not delete delayed tasks. > But they are deleted eventually (i.e. with ~TaskScheduler()), right?. Mention > that here, something like: > > // TaskScheduler deletes unexecuted delayed tasks as part of ~TaskScheduler() > instead of TaskScheduler::FlushForTesting().
fdoray@chromium.org changed reviewers: + danakj@chromium.org
danakj@: PTAL https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:900: TaskScheduler::GetInstance()->FlushForTesting(); On 2016/09/23 15:44:12, gab wrote: > How about keeping this method only for non-redirection and doing the redirection > in SequencedWorkerPool::FlushForTesting() instead? Done. https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.h:339: // to TaskScheduler is disabled. On 2016/09/23 15:44:12, gab wrote: > s/if redirection to TaskScheduler is disabled/unless redirection to > TaskScheduler is enabled/ Done. https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2367843002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:990: // TaskScheduler::FlushForTesting() does not delete delayed tasks. On 2016/09/23 15:44:12, gab wrote: > But they are deleted eventually (i.e. with ~TaskScheduler()), right?. Mention > that here, something like: > > // TaskScheduler deletes unexecuted delayed tasks as part of ~TaskScheduler() > instead of TaskScheduler::FlushForTesting(). Done.
lgtm
https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:1603: TaskScheduler::GetInstance()->FlushForTesting(); Is it okay that this does a global flush instead of just for one worker pool? https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool.h:338: // Does not wait for delayed tasks. Delayed tasks get deleted unless Can you refer to the experiment bug in this comment? Other comments have had: TODO(gab): Remove this if http://crbug.com/622400 fails https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:992: EXPECT_TRUE(tracker()->HasOneRef()); Can you instead do: bool redirected = GetParam() == ....TO_TASK_SCHED; EXPECT_EQ(!redirected, tracker()->HasOneRef()); No if branch needed. https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:1004: TaskScheduler::GetInstance()->Shutdown(); Should we verify HasOneRef down here somewhere then?
danakj@: PTAnL https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:1603: TaskScheduler::GetInstance()->FlushForTesting(); On 2016/09/23 20:33:16, danakj wrote: > Is it okay that this does a global flush instead of just for one worker pool? This is expected. Mentioned it in the comment. https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool.h:338: // Does not wait for delayed tasks. Delayed tasks get deleted unless On 2016/09/23 20:33:16, danakj wrote: > Can you refer to the experiment bug in this comment? Other comments have had: > TODO(gab): Remove this if http://crbug.com/622400 fails Done. https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:992: EXPECT_TRUE(tracker()->HasOneRef()); On 2016/09/23 20:33:16, danakj wrote: > Can you instead do: > > bool redirected = GetParam() == ....TO_TASK_SCHED; > EXPECT_EQ(!redirected, tracker()->HasOneRef()); > > No if branch needed. Done. https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:1004: TaskScheduler::GetInstance()->Shutdown(); On 2016/09/23 20:33:16, danakj wrote: > Should we verify HasOneRef down here somewhere then? Done.
LGTM https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2367843002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:1603: TaskScheduler::GetInstance()->FlushForTesting(); On 2016/09/23 20:58:17, fdoray wrote: > On 2016/09/23 20:33:16, danakj wrote: > > Is it okay that this does a global flush instead of just for one worker pool? > > This is expected. Mentioned it in the comment. Ok good, wanted to make sure you thought about it. https://codereview.chromium.org/2367843002/diff/40001/base/threading/sequence... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2367843002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:274: bool RedirectedToTaskScheduler() const { nice :) https://codereview.chromium.org/2367843002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:1011: /// Verify that all tasks are deleted once the SequencedWorkerPool and the // not /// ?
https://codereview.chromium.org/2367843002/diff/40001/base/threading/sequence... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2367843002/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:1011: /// Verify that all tasks are deleted once the SequencedWorkerPool and the On 2016/09/23 21:14:44, danakj wrote: > // not /// ? Done.
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, robliao@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2367843002/#ps60001 (title: "CR danakj #10")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by fdoray@chromium.org
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by fdoray@chromium.org
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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Support SequencedWorkerPool::FlushForTesting() with redirection to TaskScheduler. BUG=622400 ========== to ========== Support SequencedWorkerPool::FlushForTesting() with redirection to TaskScheduler. BUG=622400 Committed: https://crrev.com/bca8090714d6ca08679a3bc6b3407e280acd723e Cr-Commit-Position: refs/heads/master@{#421194} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bca8090714d6ca08679a3bc6b3407e280acd723e Cr-Commit-Position: refs/heads/master@{#421194} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
