Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2389923004:
Allow BLOCK_SHUTDOWN tasks during shutdown with SequencedWorkerPool redirected to TaskScheduler. (Closed)
DescriptionAllow BLOCK_SHUTDOWN tasks during shutdown with SequencedWorkerPool redirected to TaskScheduler.
Before this CL, any task posted to a SequencedWorkerPool redirected to
TaskScheduler after
SequencedWorkerPool::Shutdown(int max_new_blocking_tasks_after_shutdown)
returned was ignored.
With this CL, |max_new_blocking_tasks_after_shutdown| BLOCK_SHUTDOWN
tasks are allowed to be posted after SequencedWorkerPool::Shutdown
returns.
BUG=622400
Committed: https://crrev.com/387159105445bc8153931a02df261059b95b6a97
Cr-Commit-Position: refs/heads/master@{#422775}
Patch Set 1 #
Total comments: 8
Patch Set 2 : CR gab #5 #
Messages
Total messages: 18 (5 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
lgtm https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:960: UMA_HISTOGRAM_TIMES("SequencedWorkerPool.ShutdownDelayTime", Not required for this CL: Do we care that this histogram doesn't make it when redirection is on?
https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:960: UMA_HISTOGRAM_TIMES("SequencedWorkerPool.ShutdownDelayTime", On 2016/10/03 19:58:35, robliao wrote: > Not required for this CL: Do we care that this histogram doesn't make it when > redirection is on? If we care about this, we should have a similar histogram in TaskScheduler (one that we would track even after SequencedWorkerPool is deprecated).
lgtm % nit https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:960: UMA_HISTOGRAM_TIMES("SequencedWorkerPool.ShutdownDelayTime", On 2016/10/03 19:58:35, robliao wrote: > Not required for this CL: Do we care that this histogram doesn't make it when > redirection is on? I think it makes more sense that it doesn't make it as it wouldn't mean anything. https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_unittest.cc (left): https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:677: pool()->Shutdown(kNumWorkerThreads + 1); Isn't the number of "new" blocking tasks in this test just 1? I think this is allowing more than it intends (|kNumWorkerThreads + 1| is only correct for WaitUntilTasksComplete()). https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:614: // additional tasks when run, PostAdditionalTasks attemtps to post 3 "attempts"
https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:960: UMA_HISTOGRAM_TIMES("SequencedWorkerPool.ShutdownDelayTime", On 2016/10/03 20:13:41, fdoray wrote: > On 2016/10/03 19:58:35, robliao wrote: > > Not required for this CL: Do we care that this histogram doesn't make it when > > redirection is on? > > If we care about this, we should have a similar histogram in TaskScheduler (one > that we would track even after SequencedWorkerPool is deprecated). That I think we indeed should.
On 2016/10/03 20:17:16, gab wrote: > https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... > File base/threading/sequenced_worker_pool.cc (right): > > https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... > base/threading/sequenced_worker_pool.cc:960: > UMA_HISTOGRAM_TIMES("SequencedWorkerPool.ShutdownDelayTime", > On 2016/10/03 20:13:41, fdoray wrote: > > On 2016/10/03 19:58:35, robliao wrote: > > > Not required for this CL: Do we care that this histogram doesn't make it > when > > > redirection is on? > > > > If we care about this, we should have a similar histogram in TaskScheduler > (one > > that we would track even after SequencedWorkerPool is deprecated). > > That I think we indeed should. ok, I'll add an histogram in a separate CL
fdoray@chromium.org changed reviewers: + danakj@chromium.org
PTAL
danakj@: PTAL
LGTM
https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_unittest.cc (left): https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:677: pool()->Shutdown(kNumWorkerThreads + 1); On 2016/10/03 20:16:43, gab wrote: > Isn't the number of "new" blocking tasks in this test just 1? I think this is > allowing more than it intends (|kNumWorkerThreads + 1| is only correct for > WaitUntilTasksComplete()). You're right. Done. https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2389923004/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:614: // additional tasks when run, PostAdditionalTasks attemtps to post 3 On 2016/10/03 20:16:43, gab wrote: > "attempts" Done.
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, danakj@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2389923004/#ps20001 (title: "CR gab #5")
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 #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Allow BLOCK_SHUTDOWN tasks during shutdown with SequencedWorkerPool redirected to TaskScheduler. Before this CL, any task posted to a SequencedWorkerPool redirected to TaskScheduler after SequencedWorkerPool::Shutdown(int max_new_blocking_tasks_after_shutdown) returned was ignored. With this CL, |max_new_blocking_tasks_after_shutdown| BLOCK_SHUTDOWN tasks are allowed to be posted after SequencedWorkerPool::Shutdown returns. BUG=622400 ========== to ========== Allow BLOCK_SHUTDOWN tasks during shutdown with SequencedWorkerPool redirected to TaskScheduler. Before this CL, any task posted to a SequencedWorkerPool redirected to TaskScheduler after SequencedWorkerPool::Shutdown(int max_new_blocking_tasks_after_shutdown) returned was ignored. With this CL, |max_new_blocking_tasks_after_shutdown| BLOCK_SHUTDOWN tasks are allowed to be posted after SequencedWorkerPool::Shutdown returns. BUG=622400 Committed: https://crrev.com/387159105445bc8153931a02df261059b95b6a97 Cr-Commit-Position: refs/heads/master@{#422775} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/387159105445bc8153931a02df261059b95b6a97 Cr-Commit-Position: refs/heads/master@{#422775} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
