Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2285633003:
Test SequencedWorkerPool with redirection to the TaskScheduler. (Closed)
DescriptionTest SequencedWorkerPool with redirection to the TaskScheduler.
This CL makes most SequencedWorkerPoolTest a TestWithParam. The parameter
controls whether redirection to TaskScheduler is enabled.
BUG=622400
Committed: https://crrev.com/37e79479d334a1a18cdf5918255f1531bc11afd9
Cr-Commit-Position: refs/heads/master@{#419322}
Patch Set 1 #
Total comments: 12
Patch Set 2 : CR robliao #3 #Patch Set 3 : self-review #
Total comments: 6
Patch Set 4 : CR robliao #7 #
Total comments: 14
Patch Set 5 : Rewrote everything #
Total comments: 18
Patch Set 6 : CR gab/danakj #18-19 #
Messages
Total messages: 27 (11 generated)
fdoray@chromium.org changed reviewers: + danakj@chromium.org, robliao@chromium.org
PTAL https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:1029: // when tasks are redirected to the TaskScheduler. It will never be supported TODO(fdoray): Remove call from chromeos/accelerometer/accelerometer_reader.cc
https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:546: std::map<int, TaskRunnerAndShutdownBehavior> sequenced_task_runner_map_; Unrelated Optional: Should these be unordered_maps? https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:755: TaskScheduler::GetInstance()->Shutdown(); Does this mean that Shutdown doesn't quite work when tasks are redirected without Shutting down the TaskScheduler as well? https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:1069: constexpr size_t kNumThreads = 10; This might be more useful as a global. Is there a good reason to vary the number of threads between the runners in the future? https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:1090: pool_owner_->pool()->Shutdown(); Add a comment why Start/Stop isn't symmetric (why we only shutdown the pool if redirection is off) https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:1150: // not run, they're simply deleted. Same here
Description was changed from ========== Test SequencedWorkerPool with redirection to the TaskScheduler. This CL makes most SequencedWorkerPool tests run with redirection to the TaskScheduler is enabled. It also improves the redirection of SequencedWorkerPool tasks to the TaskScheduler: - Delays are honored. - DCHECK() when tasks are posted to the same sequence with different shutdown behaviors. - DCHECK() when tasks are posted to a given single-threaded pool with different shutdown behaviors or to different sequences. BUG=622400 ========== to ========== Test SequencedWorkerPool with redirection to the TaskScheduler. This CL makes most SequencedWorkerPool tests run with redirection to the TaskScheduler is enabled. It also improves the redirection of SequencedWorkerPool tasks to the TaskScheduler: - Delays are honored. - DCHECK() when tasks are posted to the same sequence with different shutdown behaviors. - DCHECK() when tasks are posted to a given single-threaded pool with different shutdown behaviors or to different sequences. - Handle shutdown behavior in the TaskScheduler only rather than in both SequencedWorkerPool and TaskScheduler. BUG=622400 ==========
Description was changed from ========== Test SequencedWorkerPool with redirection to the TaskScheduler. This CL makes most SequencedWorkerPool tests run with redirection to the TaskScheduler is enabled. It also improves the redirection of SequencedWorkerPool tasks to the TaskScheduler: - Delays are honored. - DCHECK() when tasks are posted to the same sequence with different shutdown behaviors. - DCHECK() when tasks are posted to a given single-threaded pool with different shutdown behaviors or to different sequences. - Handle shutdown behavior in the TaskScheduler only rather than in both SequencedWorkerPool and TaskScheduler. BUG=622400 ========== to ========== Test SequencedWorkerPool with redirection to the TaskScheduler. This CL makes most SequencedWorkerPool tests run with redirection to the TaskScheduler is enabled. It also improves the redirection of SequencedWorkerPool tasks to the TaskScheduler: - Delays are honored. - DCHECK() when tasks are posted to the same sequence with different shutdown behaviors. - DCHECK() when tasks are posted to a given single-threaded pool with different shutdown behaviors or to different sequences. - Make SequencedWorkerPool::Shutdown a no-op when redirection to the TaskScheduler is enabled. Handle shutdown behavior in TaskScheduler only rather than in both SequencedWorkerPool and TaskScheduler. BUG=622400 ==========
robliao@: PTAnL https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:546: std::map<int, TaskRunnerAndShutdownBehavior> sequenced_task_runner_map_; On 2016/08/27 00:28:59, robliao wrote: > Unrelated Optional: Should these be unordered_maps? Done. https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:755: TaskScheduler::GetInstance()->Shutdown(); On 2016/08/27 00:28:59, robliao wrote: > Does this mean that Shutdown doesn't quite work when tasks are redirected > without Shutting down the TaskScheduler as well? Added comment to clarify. https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:1029: // when tasks are redirected to the TaskScheduler. It will never be supported On 2016/08/26 13:44:16, fdoray wrote: > TODO(fdoray): Remove call from chromeos/accelerometer/accelerometer_reader.cc Done. https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:1069: constexpr size_t kNumThreads = 10; On 2016/08/27 00:29:00, robliao wrote: > This might be more useful as a global. Is there a good reason to vary the number > of threads between the runners in the future? Done. https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:1090: pool_owner_->pool()->Shutdown(); On 2016/08/27 00:28:59, robliao wrote: > Add a comment why Start/Stop isn't symmetric (why we only shutdown the pool if > redirection is off) Changed the code to always shut down the pool as it is always valid to do so. https://codereview.chromium.org/2285633003/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:1150: // not run, they're simply deleted. On 2016/08/27 00:28:59, robliao wrote: > Same here Done.
lgtm + Comments https://codereview.chromium.org/2285633003/diff/40001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2285633003/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:779: } else { Nit: Remove else https://codereview.chromium.org/2285633003/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:850: // Single-threaded pools can legitimatelly assume thread affinity. Disallow Nit: s/legitimatelly/legitimately/ https://codereview.chromium.org/2285633003/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:854: (unsequenced_task_runners_.size() + A caller could plausibly call GetTaskRunnerWithShutdownBehavior with the same shutdown behavior. Do we want to disallow those?
Description was changed from ========== Test SequencedWorkerPool with redirection to the TaskScheduler. This CL makes most SequencedWorkerPool tests run with redirection to the TaskScheduler is enabled. It also improves the redirection of SequencedWorkerPool tasks to the TaskScheduler: - Delays are honored. - DCHECK() when tasks are posted to the same sequence with different shutdown behaviors. - DCHECK() when tasks are posted to a given single-threaded pool with different shutdown behaviors or to different sequences. - Make SequencedWorkerPool::Shutdown a no-op when redirection to the TaskScheduler is enabled. Handle shutdown behavior in TaskScheduler only rather than in both SequencedWorkerPool and TaskScheduler. BUG=622400 ========== to ========== Test SequencedWorkerPool with redirection to the TaskScheduler. This CL makes most SequencedWorkerPool tests run with redirection to the TaskScheduler is enabled. It also improves the redirection of SequencedWorkerPool tasks to the TaskScheduler: - Delays are honored. - DCHECK() when tasks are posted to the same sequence with different shutdown behaviors. - DCHECK() when tasks are posted to a given single-threaded pool with different shutdown behaviors. - Make SequencedWorkerPool::Shutdown a no-op when redirection to the TaskScheduler is enabled. Handle shutdown behavior in TaskScheduler only rather than in both SequencedWorkerPool and TaskScheduler. BUG=622400 ==========
fdoray@chromium.org changed reviewers: + jochen@chromium.org
danakj@: Please review changes in base/ jochen@: Please review changes in chrome/browser/ https://codereview.chromium.org/2285633003/diff/40001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2285633003/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:779: } else { On 2016/08/29 23:28:50, robliao wrote: > Nit: Remove else Done. https://codereview.chromium.org/2285633003/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:850: // Single-threaded pools can legitimatelly assume thread affinity. Disallow On 2016/08/29 23:28:50, robliao wrote: > Nit: s/legitimatelly/legitimately/ Done. https://codereview.chromium.org/2285633003/diff/40001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:854: (unsequenced_task_runners_.size() + On 2016/08/29 23:28:50, robliao wrote: > A caller could plausibly call GetTaskRunnerWithShutdownBehavior with the same > shutdown behavior. Do we want to disallow those? No, we shouldn't disallow that. Done.
lgtm
danakj@: PTAL
https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:875: return RunsTasksOnCurrentThreadAssertSynchronized(); The non-task runner code would return false if the token was invalid wouldnt it? Cuz it wouldnt be equal to what is currently running? Or could that one be invalid and equal? https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:877: // TODO(gab): This currently only verifies that the current thread is a This TODO applies to the above also right https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:1444: g_all_pools_state = AllPoolsState::NONE_ACTIVE; DCHECK that it's REDIRECTED_TO_TASK_SCHEDULER currently? https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:407: // task has not only not run, but has also been destroyed. The TaskScheduler This isn't checking that the task was run tho with the scheduler? https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:508: single_threaded_pool.pool()->Shutdown(); so i noticed that earlier test did ResetPool() instead of this Shutdown.. why are they different here/there? https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:819: // enabled. For each of these tests that are being skipped. Can you point at a TaskScheduler unit test that covers the use case? Or are there actually not any? https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:905: // these tests out of the conditional. Is there a bug to point at? https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:1286: // Don't run TaskRunnerTest, SequencedTaskRunnerTest and It feels a bit weird to make this call here, instead of skipping individual tests for them like you did in this file?
gab@chromium.org changed reviewers: + gab@chromium.org
Thanks for following up on https://codereview.chromium.org/2241703002 while I was away :-). I think a few things should be fixed as precursor CLs while we figure out the test story (haven't reviewed that yet). I also think we should just ban single-threaded SWP. I'll get started on that. https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:699: g_all_pools_state != AllPoolsState::REDIRECTED_TO_TASK_SCHEDULER) { Good catch, trunk uses atomics for this now though. https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:775: if (g_all_pools_state == AllPoolsState::REDIRECTED_TO_TASK_SCHEDULER) { atomic https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:820: if (max_threads_ == 1) { I'm thinking we might just want to disallow pools with only one thread. The only remaining ones are "WebCrypto" and image_fetcher_unittest.mm. Doing so would much simplify the redirection and I don't think there's a valid reason to use single-threaded SWP now that we have non-joinable Threads. I can do this if we agree. https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:831: single_threaded_task_runner_.shutdown_behavior); Different sequences on single-threaded pool could have different shutdown behaviors though, right? https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:1444: g_all_pools_state = AllPoolsState::NONE_ACTIVE; On 2016/09/01 23:16:34, danakj wrote: > DCHECK that it's REDIRECTED_TO_TASK_SCHEDULER currently? And use atomics now. https://codereview.chromium.org/2285633003/diff/60001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:1586: DCHECK_NE(AllPoolsState::REDIRECTED_TO_TASK_SCHEDULER, g_all_pools_state); Atomics
Description was changed from ========== Test SequencedWorkerPool with redirection to the TaskScheduler. This CL makes most SequencedWorkerPool tests run with redirection to the TaskScheduler is enabled. It also improves the redirection of SequencedWorkerPool tasks to the TaskScheduler: - Delays are honored. - DCHECK() when tasks are posted to the same sequence with different shutdown behaviors. - DCHECK() when tasks are posted to a given single-threaded pool with different shutdown behaviors. - Make SequencedWorkerPool::Shutdown a no-op when redirection to the TaskScheduler is enabled. Handle shutdown behavior in TaskScheduler only rather than in both SequencedWorkerPool and TaskScheduler. BUG=622400 ========== to ========== Test SequencedWorkerPool with redirection to the TaskScheduler. This CL makes most SequencedWorkerPoolTest a TestWithParam. The parameter controls whether redirection to TaskScheduler is enabled. BUG=622400 ==========
PTAL I didn't respond to comments on the previous patch set since I rewrote everything.
Mostly lgtm % comments/questions, thanks :-)! https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:1476: // (can usually not be called after a worker has been created). "when current state is WORKER_CREATED" and "usually not after a worker has been created" are contradictory, no? https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:278: void DeletePool() { pool_owner_ = nullptr; } pool_owner_.reset() https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:284: ->JoinForTesting(); Should we add TaskScheduler::JoinForTesting? https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:913: // Note: The TaskScheduler does not destroy tasks on shutdown. Why does it pass in redirection then?! Are the DestructionDeadlockChecker not working as intended? https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:932: // during shutdown. I don't understand what this test has to do with the "limit of # BLOCK_SHUTDOWN tasks"?
LGTM % gab https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:58: // should only ever transition from NONE_ACTIVE to the active states, to one of the active states. Transitions between active states (you have "actives states") https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:255: base::Bind([](const TaskTraits&) -> size_t { return 0U; })); do you need the -> size_t here? something like it gets inferred from the return type https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:262: // Destroy the SequencedWorkerPool before the TaskScheduler. say why instead of just what https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:277: // down, and creates a new instance. comment wrong now
https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:58: // should only ever transition from NONE_ACTIVE to the active states, On 2016/09/16 20:29:36, danakj wrote: > to one of the active states. Transitions between active states > > (you have "actives states") Done. https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:1476: // (can usually not be called after a worker has been created). On 2016/09/16 20:16:10, gab wrote: > "when current state is WORKER_CREATED" and "usually not after a worker has been > created" are contradictory, no? Made the comment clearer. https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:255: base::Bind([](const TaskTraits&) -> size_t { return 0U; })); On 2016/09/16 20:29:36, danakj wrote: > do you need the -> size_t here? something like it gets inferred from the return > type doesn't compile without the -> size_t https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:262: // Destroy the SequencedWorkerPool before the TaskScheduler. On 2016/09/16 20:29:36, danakj wrote: > say why instead of just what Done. https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:277: // down, and creates a new instance. On 2016/09/16 20:29:36, danakj wrote: > comment wrong now Done. https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:278: void DeletePool() { pool_owner_ = nullptr; } On 2016/09/16 20:16:10, gab wrote: > pool_owner_.reset() Done. https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:284: ->JoinForTesting(); On 2016/09/16 20:16:10, gab wrote: > Should we add TaskScheduler::JoinForTesting? I don't think we want every test that needs a TaskScheduler do all these steps (register task scheduler, join for testing, setinstance(nullptr)). Instead, we should have a ScopedTaskScheduler that people just instantiate in their test to automatically register, join and unregister. https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:913: // Note: The TaskScheduler does not destroy tasks on shutdown. On 2016/09/16 20:16:10, gab wrote: > Why does it pass in redirection then?! Are the DestructionDeadlockChecker not > working as intended? DestructionDeadlockChecker verifies that using the SequencedWorkerPool when the task is destructed doesn't cause a deadlock... and it doesn't. I changed the comment. https://codereview.chromium.org/2285633003/diff/80001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:932: // during shutdown. On 2016/09/16 20:16:10, gab wrote: > I don't understand what this test has to do with the "limit of # BLOCK_SHUTDOWN > tasks"? Clarified the comment.
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, jochen@chromium.org, danakj@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2285633003/#ps100001 (title: "CR gab/danakj #18-19")
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 ========== Test SequencedWorkerPool with redirection to the TaskScheduler. This CL makes most SequencedWorkerPoolTest a TestWithParam. The parameter controls whether redirection to TaskScheduler is enabled. BUG=622400 ========== to ========== Test SequencedWorkerPool with redirection to the TaskScheduler. This CL makes most SequencedWorkerPoolTest a TestWithParam. The parameter controls whether redirection to TaskScheduler is enabled. BUG=622400 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Test SequencedWorkerPool with redirection to the TaskScheduler. This CL makes most SequencedWorkerPoolTest a TestWithParam. The parameter controls whether redirection to TaskScheduler is enabled. BUG=622400 ========== to ========== Test SequencedWorkerPool with redirection to the TaskScheduler. This CL makes most SequencedWorkerPoolTest a TestWithParam. The parameter controls whether redirection to TaskScheduler is enabled. BUG=622400 Committed: https://crrev.com/37e79479d334a1a18cdf5918255f1531bc11afd9 Cr-Commit-Position: refs/heads/master@{#419322} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/37e79479d334a1a18cdf5918255f1531bc11afd9 Cr-Commit-Position: refs/heads/master@{#419322} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
