Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1701343003:
TaskScheduler [13] TaskSchedulerImpl (Closed)
DescriptionTaskScheduler [13] TaskSchedulerImpl
TaskSchedulerImpl is the default implementation of the TaskScheduler
interface. It runs tasks in 4 thread pools:
- Background with file i/o
- Background without file i/o
- Normal with file i/o
- Normal without file i/o
This change completes the necessary changes described in
https://codereview.chromium.org/1698183005/
BUG=553459
Committed: https://crrev.com/4151ceaeb5627ddb2fd8c236c8fe055355c4c9a0
Cr-Commit-Position: refs/heads/master@{#392063}
Patch Set 1 #Patch Set 2 : initial patch for review #
Total comments: 34
Patch Set 3 : rebase #Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : CR gab #11 #
Total comments: 1
Patch Set 7 : self review #
Total comments: 14
Patch Set 8 : rebase #Patch Set 9 : rebase (includes implementation of PostTaskWithTraits) #Patch Set 10 : CR gab #13 #
Total comments: 2
Patch Set 11 : rebase + add SequenceSortKey::priority() #
Total comments: 6
Patch Set 12 : CR robliao #
Total comments: 3
Patch Set 13 : auto -> std::unique_ptr<TaskSchedulerImpl> #
Total comments: 6
Patch Set 14 : rebase + CR danakj/gab #Patch Set 15 : fix linux build error #Patch Set 16 : fix ios build error #
Messages
Total messages: 45 (18 generated)
Description was changed from ========== TaskScheduler [8/8] TaskSchedulerImpl This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [8/8] TaskSchedulerImpl This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ==========
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
Can you review this CL? Thanks.
Description was changed from ========== TaskScheduler [8/8] TaskSchedulerImpl This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [9/9] TaskSchedulerImpl This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ==========
On 2016/02/17 20:23:56, fdoray wrote: > Can you review this CL? Thanks. I assume this CL needs rebasing once the current 3 CLs land? We probably also want to wait until other core functionality has landed (e.g. service thread, child task runner, etc.)? At least for post_task.h. Let us know when we should take a look :-).
Description was changed from ========== TaskScheduler [9/9] TaskSchedulerImpl This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [11] TaskSchedulerImpl This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Description was changed from ========== TaskScheduler [11] TaskSchedulerImpl This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [14] TaskSchedulerImpl This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Description was changed from ========== TaskScheduler [14] TaskSchedulerImpl This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [13] TaskSchedulerImpl This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Description was changed from ========== TaskScheduler [13] TaskSchedulerImpl This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [13] TaskSchedulerImpl TaskSchedulerImpl is the default implementation of the TaskScheduler interface. It runs tasks in 4 thread pools: - Background with file i/o - Background without file i/o - Normal with file i/o - Normal without file i/o This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ==========
gab@/robliao@: Can you review this CL? Thanks.
Woot, getting there :-)! Looks great, few comments: https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_thread_pool_impl_unittest.cc (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_thread_pool_impl_unittest.cc:223: // Post tasks to keep all threads busy except one until |event| is signaled. s/Post tasks/Post blocking tasks/ ? (since it's now no longer implicit in the TestTaskFactory that it will block, it's instead a side-effect of the Closure passed to it here) https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_thread_pool_impl_unittest.cc:257: // tasks are added to different sequences and can run simultaneously when the s/tasks/the blocking tasks/ https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:22: TaskScheduler() = default; Move to previous CL introducing this file? https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:51: When you rebase on latest version of this interface, have task_scheduler.cc's InitializeDefaultTaskScheduler() impl create a TaskSchedulerImpl. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:22: std::unique_ptr<TaskSchedulerImpl> TaskSchedulerImpl::Create() { In SchedulerThreadPool we had a static Create() with private constructor to support return failure. This class doesn't support creation failure so I think we should just use a public constructor with no Create(). https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:112: GetThreadPoolForTraits(traits)->ReEnqueueSequence(std::move(sequence), Why not use |next_task_in_sequence->traits| instead of creating a new |traits| here? Feels weird to simulate just the required traits (i.e. this code will need to change if GetThreadPoolForTraits()'s decision ever evolves to depending on more traits.. I would see this entire method as : const SequenceSortKey sort_key = sequence->GetSortKey(); GetThreadPoolForTraits(sequence->PeekTask().traits) ->ReEnqueueSequence(std::move(sequence), sort_key); https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:33: static std::unique_ptr<TaskSchedulerImpl> Create(); // Creates and returns an initialized TaskSchedulerImpl. CHECKs on failure to do so (never returns null). https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:73: WaitableEvent join_for_testing_returned_; #if DCHECK_IS_ON() https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:48: std::unique_ptr<TaskSchedulerImpl> scheduler_; If we get rid of Create() this can just be a plain member (no ptr) and we can remove SetUp() :-) https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:54: std::vector<TaskSchedulerImplTestParams> GetTaskSchedulerImplTestParams() { // Returns a vector with a TaskSchedulerImplTestParams for each valid combination of {ExecutionMode, TaskPriority, WithFileIO()}. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:57: // TODO(fdoray): Add ExecutionMode::SINGLE_THREADED once it's supported. Isn't this based on top of the CL that makes it supported? https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:61: for (size_t execution_mode_index = 0; for (ExecutionMode execution_mode : execution_modes) https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:96: Bind(&ExpectThreadPriority, Also add AssertIOAllowed expectations (to verify WithFileIO()) in the verification task (ExecutionMode will be verified by TestTaskFactory already after the thread-handles change so that's okay -- probably worth adding a comment to that effect). (requires dependency on your incoming CL but that's okay IMO) https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:104: One of the key part of TaskSchedulerImpl (well I guess SchedulerThreadPool too but we didn't test that on it either IIRC) is to be able to post tasks of all combinations at once and have them land where they belong. I think it's good to test each combination in isolation, but I'd also like to see a test that spams tasks from all combinations in parallel and ensures they all end up where they belong (via the enhanced assert described in my previous comment). https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:107: ::testing::ValuesIn(GetTaskSchedulerImplTestParams())); Whatttt!!!?!?! This doesn't have to be statically generated?! I didn't know that :-)! Is gtest_filter happy with this from command-line (can you name a specific failing combination to deterministically try the same one again -- I guess so since the index is deterministic but still that's a very awesome trick, I'd previously resolved to try to define all values here inline with sometimes complex combinations of testing::Combine)?
gab@: Done. Can you take another look? robliao@: Can you review this CL? Thanks! https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_thread_pool_impl_unittest.cc (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_thread_pool_impl_unittest.cc:223: // Post tasks to keep all threads busy except one until |event| is signaled. On 2016/04/27 19:15:28, gab wrote: > s/Post tasks/Post blocking tasks/ ? (since it's now no longer implicit in the > TestTaskFactory that it will block, it's instead a side-effect of the Closure > passed to it here) Done. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_thread_pool_impl_unittest.cc:257: // tasks are added to different sequences and can run simultaneously when the On 2016/04/27 19:15:28, gab wrote: > s/tasks/the blocking tasks/ Done. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:22: TaskScheduler() = default; On 2016/04/27 19:15:28, gab wrote: > Move to previous CL introducing this file? No longer needed without the DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:51: On 2016/04/27 19:15:28, gab wrote: > When you rebase on latest version of this interface, have task_scheduler.cc's > InitializeDefaultTaskScheduler() impl create a TaskSchedulerImpl. Done. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:22: std::unique_ptr<TaskSchedulerImpl> TaskSchedulerImpl::Create() { On 2016/04/27 19:15:28, gab wrote: > In SchedulerThreadPool we had a static Create() with private constructor to > support return failure. This class doesn't support creation failure so I think > we should just use a public constructor with no Create(). Done. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:112: GetThreadPoolForTraits(traits)->ReEnqueueSequence(std::move(sequence), On 2016/04/27 19:15:28, gab wrote: > Why not use |next_task_in_sequence->traits| instead of creating a new |traits| > here? Feels weird to simulate just the required traits (i.e. this code will need > to change if GetThreadPoolForTraits()'s decision ever evolves to depending on > more traits.. > > I would see this entire method as : > > const SequenceSortKey sort_key = sequence->GetSortKey(); > GetThreadPoolForTraits(sequence->PeekTask().traits) > ->ReEnqueueSequence(std::move(sequence), sort_key); Added comment to explain why I do this. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:33: static std::unique_ptr<TaskSchedulerImpl> Create(); On 2016/04/27 19:15:28, gab wrote: > // Creates and returns an initialized TaskSchedulerImpl. CHECKs on failure to do > so (never returns null). Removed this method. Added a similar comment on the constructor. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:73: WaitableEvent join_for_testing_returned_; On 2016/04/27 19:15:28, gab wrote: > #if DCHECK_IS_ON() Done. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:48: std::unique_ptr<TaskSchedulerImpl> scheduler_; On 2016/04/27 19:15:28, gab wrote: > If we get rid of Create() this can just be a plain member (no ptr) and we can > remove SetUp() :-) Done. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:54: std::vector<TaskSchedulerImplTestParams> GetTaskSchedulerImplTestParams() { On 2016/04/27 19:15:29, gab wrote: > // Returns a vector with a TaskSchedulerImplTestParams for each valid > combination of {ExecutionMode, TaskPriority, WithFileIO()}. Done. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:57: // TODO(fdoray): Add ExecutionMode::SINGLE_THREADED once it's supported. On 2016/04/27 19:15:29, gab wrote: > Isn't this based on top of the CL that makes it supported? It is now :) https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:61: for (size_t execution_mode_index = 0; On 2016/04/27 19:15:28, gab wrote: > for (ExecutionMode execution_mode : execution_modes) Done. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:96: Bind(&ExpectThreadPriority, On 2016/04/27 19:15:29, gab wrote: > Also add AssertIOAllowed expectations (to verify WithFileIO()) in the > verification task (ExecutionMode will be verified by TestTaskFactory already > after the thread-handles change so that's okay -- probably worth adding a > comment to that effect). > > (requires dependency on your incoming CL but that's okay IMO) Done. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:104: On 2016/04/27 19:15:28, gab wrote: > One of the key part of TaskSchedulerImpl (well I guess SchedulerThreadPool too > but we didn't test that on it either IIRC) is to be able to post tasks of all > combinations at once and have them land where they belong. > > I think it's good to test each combination in isolation, but I'd also like to > see a test that spams tasks from all combinations in parallel and ensures they > all end up where they belong (via the enhanced assert described in my previous > comment). Done. https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:107: ::testing::ValuesIn(GetTaskSchedulerImplTestParams())); On 2016/04/27 19:15:28, gab wrote: > Whatttt!!!?!?! This doesn't have to be statically generated?! I didn't know that > :-)! Is gtest_filter happy with this from command-line (can you name a specific > failing combination to deterministically try the same one again -- I guess so > since the index is deterministic but still that's a very awesome trick, I'd > previously resolved to try to define all values here inline with sometimes > complex combinations of testing::Combine)? The name of a test is TaskTraitsExecutionModeCombinations/TaskSchedulerImplTest.PostTasks/#, where # is an index in the vector returned by GetTaskSchedulerImplTestParams() (indexes don't change between runs). https://codereview.chromium.org/1701343003/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:19: : delayed_task_manager_(Bind(&DoNothing)) Yes, this is the result of git cl format.
lvg https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:107: ::testing::ValuesIn(GetTaskSchedulerImplTestParams())); On 2016/04/28 18:36:32, fdoray wrote: > On 2016/04/27 19:15:28, gab wrote: > > Whatttt!!!?!?! This doesn't have to be statically generated?! I didn't know > that > > :-)! Is gtest_filter happy with this from command-line (can you name a > specific > > failing combination to deterministically try the same one again -- I guess so > > since the index is deterministic but still that's a very awesome trick, I'd > > previously resolved to try to define all values here inline with sometimes > > complex combinations of testing::Combine)? > > The name of a test is > TaskTraitsExecutionModeCombinations/TaskSchedulerImplTest.PostTasks/#, where # > is an index in the vector returned by GetTaskSchedulerImplTestParams() (indexes > don't change between runs). Cool :-), I guess if one of them was flaky the flakiness dashboard would have a hard time keeping track when this test evolves and indices change, but that's probably not an issue for this one in practice ;-). https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:32: DCHECK(join_for_testing_returned_.IsSignaled()); DLOG_ASSERT?! :-) https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:116: // with the highest priority in |sequence|. s/./as opposed to the next task's specific priority/ https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:57: // Windows. I wouldn't even mention why we don't use EXPECT_DCHECK_DEATH as I don't think this code would be prettier if we did use it anyways :-). Also, is EXPECT_DCHECK_DEATH really slow enough to justify not using it in a test when it'd be a cleaner choice though? https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:63: void VerifyTaskEnvironement(const TaskTraits& traits) { Add a comment about what this verifies and also add something to the effect of : "note: ExecutionMode is verified inside TestTaskFactory" https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:64: EXPECT_EQ((traits.priority() == TaskPriority::BACKGROUND) Remove extra () for == check (the ternary operator is pretty much last in operator precedence): http://en.cppreference.com/w/cpp/language/operator_precedence https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:71: // !ENABLE_THREAD_RESTRICTIONS. Instead return true from GetIOAllowed() when #if ENABLE_THREAD_RESTRICTIONS? https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:179: // TODO(fdoray): Add tests with Sequences that move around thread pools once Please add an item for this in our internal tracker spreadsheet.
gab@: Done. Can you take another look? robliao@: Can you review this CL? Thanks. https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:32: DCHECK(join_for_testing_returned_.IsSignaled()); On 2016/04/28 19:29:02, gab wrote: > DLOG_ASSERT?! :-) Done. https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:116: // with the highest priority in |sequence|. On 2016/04/28 19:29:02, gab wrote: > s/./as opposed to the next task's specific priority/ Done. https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:57: // Windows. On 2016/04/28 19:29:02, gab wrote: > I wouldn't even mention why we don't use EXPECT_DCHECK_DEATH as I don't think > this code would be prettier if we did use it anyways :-). > > Also, is EXPECT_DCHECK_DEATH really slow enough to justify not using it in a > test when it'd be a cleaner choice though? Removed comment. TaskSchedulerImplTest.PostTasks runs 150 tasks. After 15 seconds, I thought it was blocked, but it was just spending a lot of time in EXPECT_DCHECK_DEATH. https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:63: void VerifyTaskEnvironement(const TaskTraits& traits) { On 2016/04/28 19:29:02, gab wrote: > Add a comment about what this verifies and also add something to the effect of : > "note: ExecutionMode is verified inside TestTaskFactory" Done. https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:64: EXPECT_EQ((traits.priority() == TaskPriority::BACKGROUND) On 2016/04/28 19:29:02, gab wrote: > Remove extra () for == check (the ternary operator is pretty much last in > operator precedence): > http://en.cppreference.com/w/cpp/language/operator_precedence Done. https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:71: // !ENABLE_THREAD_RESTRICTIONS. On 2016/04/28 19:29:02, gab wrote: > Instead return true from GetIOAllowed() when #if ENABLE_THREAD_RESTRICTIONS? Clarified comment. GetIOAllowed() already returns true all the time when ENABLE_THREAD_RESTRICTIONS. This test would fail for a task *without* file I/O when !ENABLE_THREAD_RESTRICTIONS. https://codereview.chromium.org/1701343003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:179: // TODO(fdoray): Add tests with Sequences that move around thread pools once On 2016/04/28 19:29:02, gab wrote: > Please add an item for this in our internal tracker spreadsheet. Done.
lgtm, thanks! https://codereview.chromium.org/1701343003/diff/180001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:36: "behavior of DLOG_ASSERT, have diverged."); Why does that matter? The code below doesn't depend on DCHECK_IS_ON (unlike EXPECT_DCHECK_DEATH did in that other CL) Or is it because the other code can't be behind #if ::logging::DEBUG_MODE because of ordering issues between precompiler rules & all?
robliao@: Can you review this CL? Thanks. https://codereview.chromium.org/1701343003/diff/180001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:36: "behavior of DLOG_ASSERT, have diverged."); On 2016/04/28 22:44:59, gab wrote: > Why does that matter? The code below doesn't depend on DCHECK_IS_ON (unlike > EXPECT_DCHECK_DEATH did in that other CL) > > Or is it because the other code can't be behind #if ::logging::DEBUG_MODE > because of ordering issues between precompiler rules & all? The definition of |join_for_testing_returned_| in task_scheduler_impl.h is guarded by #if DCHECK_IS_ON(). It can't be guarded by #if DEBUG_MODE because this variable is not visible by the precompiler. If ever DCHECK_IS_ON() is false and DEBUG_MODE is true, the code below won't compile. I believe that the static_assert above could help understand why.
robliao@: Can you review this CL? Thanks. In patch set 11, I added SequenceSortKey::priority() because a CL from gab@ made the members of SequenceSortKey private.
lgtm % constructor https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:22: std::unique_ptr<TaskSchedulerImpl> TaskSchedulerImpl::Create() { On 2016/04/28 18:36:32, fdoray wrote: > On 2016/04/27 19:15:28, gab wrote: > > In SchedulerThreadPool we had a static Create() with private constructor to > > support return failure. This class doesn't support creation failure so I think > > we should just use a public constructor with no Create(). > > Done. While this class doesn't support creation failure, we could leave it up to the caller to perform the CHECK instead of doing it here. https://codereview.chromium.org/1701343003/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:28: Initialize(); While we really don't support TaskSchedulerImpl creation failure, I suspect this would be classified as doing too much work in the constructor. We may have to use a Create static method to satisfy the style guide. https://codereview.chromium.org/1701343003/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:87: ThreadPriority::BACKGROUND, 1U, IORestriction::DISALLOWED, Nit: More folks seem to use the lower case version for the unsigned constant. 1U -> 1u
fdoray@chromium.org changed reviewers: + danakj@chromium.org
robliao@: 2 done, 1 pending discussion danakj: Can you review this CL? Thanks! https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:22: std::unique_ptr<TaskSchedulerImpl> TaskSchedulerImpl::Create() { On 2016/04/29 18:34:21, robliao wrote: > On 2016/04/28 18:36:32, fdoray wrote: > > On 2016/04/27 19:15:28, gab wrote: > > > In SchedulerThreadPool we had a static Create() with private constructor to > > > support return failure. This class doesn't support creation failure so I > think > > > we should just use a public constructor with no Create(). > > > > Done. > > While this class doesn't support creation failure, we could leave it up to the > caller to perform the CHECK instead of doing it here. If TaskSchedulerImpl::Create successfully creates the 3 first thread pools of a TaskSchedulerImpl but fails to create the last one, it needs to delete the 3 first thread pools before returning nullptr. Unfortunately, we currently don't support deleting a thread pool without calling its JoinForTesting() method first. I think a CHECK() as soon as we fail to create a thread pool is better than supporting deleting a thread pool in production. https://codereview.chromium.org/1701343003/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:28: Initialize(); On 2016/04/29 18:34:22, robliao wrote: > While we really don't support TaskSchedulerImpl creation failure, I suspect this > would be classified as doing too much work in the constructor. We may have to > use a Create static method to satisfy the style guide. Added a static Create method to solve the "doing to much work in the constructor" problem. https://codereview.chromium.org/1701343003/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:87: ThreadPriority::BACKGROUND, 1U, IORestriction::DISALLOWED, On 2016/04/29 18:34:22, robliao wrote: > Nit: More folks seem to use the lower case version for the unsigned constant. 1U > -> 1u Done.
https://codereview.chromium.org/1701343003/diff/220001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1701343003/diff/220001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:184: auto scheduler = TaskSchedulerImpl::Create(); I don't know how I missed this on the first pass. LeakChecker will compalin about this, so we'll need to delete this add the end or use a std::unique_ptr
danakj@: Can you review this CL? Thanks. https://codereview.chromium.org/1701343003/diff/220001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1701343003/diff/220001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:184: auto scheduler = TaskSchedulerImpl::Create(); On 2016/04/29 19:56:03, robliao wrote: > I don't know how I missed this on the first pass. LeakChecker will compalin > about this, so we'll need to delete this add the end or use a std::unique_ptr It's not a leak because TaskSchedulerImpl::Create returns an std::unique_ptr. Changed auto -> std::unique_ptr<TaskSchedulerImpl> to make this clearer.
https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:22: std::unique_ptr<TaskSchedulerImpl> TaskSchedulerImpl::Create() { On 2016/04/29 19:42:42, fdoray wrote: > On 2016/04/29 18:34:21, robliao wrote: > > On 2016/04/28 18:36:32, fdoray wrote: > > > On 2016/04/27 19:15:28, gab wrote: > > > > In SchedulerThreadPool we had a static Create() with private constructor > to > > > > support return failure. This class doesn't support creation failure so I > > think > > > > we should just use a public constructor with no Create(). > > > > > > Done. > > > > While this class doesn't support creation failure, we could leave it up to the > > caller to perform the CHECK instead of doing it here. > > If TaskSchedulerImpl::Create successfully creates the 3 first thread pools of a > TaskSchedulerImpl but fails to create the last one, it needs to delete the 3 > first thread pools before returning nullptr. Unfortunately, we currently don't > support deleting a thread pool without calling its JoinForTesting() method > first. I think a CHECK() as soon as we fail to create a thread pool is better > than supporting deleting a thread pool in production. Agreed, and I think the "doing work in the constructor" rule is meant to address inability to return failure or exceptions but here we don't want to do that anyways. https://codereview.chromium.org/1701343003/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:87: ThreadPriority::BACKGROUND, 1U, IORestriction::DISALLOWED, On 2016/04/29 19:42:42, fdoray wrote: > On 2016/04/29 18:34:22, robliao wrote: > > Nit: More folks seem to use the lower case version for the unsigned constant. > 1U > > -> 1u > > Done. Really? I've only ever since the upper-case one. Seems to be 2500 to 500 in favor of upper-case: https://code.google.com/p/chromium/codesearch#search/&q=case:yes%20%5Cb%5B0-9...
https://codereview.chromium.org/1701343003/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/1701343003/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_scheduler.cc:8: #include "base/memory/ptr_util.h" unused
https://codereview.chromium.org/1701343003/diff/220001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1701343003/diff/220001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:184: auto scheduler = TaskSchedulerImpl::Create(); On 2016/04/29 20:26:20, fdoray wrote: > On 2016/04/29 19:56:03, robliao wrote: > > I don't know how I missed this on the first pass. LeakChecker will compalin > > about this, so we'll need to delete this add the end or use a std::unique_ptr > > It's not a leak because TaskSchedulerImpl::Create returns an std::unique_ptr. > Changed auto -> std::unique_ptr<TaskSchedulerImpl> to make this clearer. Ah, right you are! std::unique_ptr would indeed be clearer.
danakj@: Can you review this CL? Thanks.
LGTM https://codereview.chromium.org/1701343003/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:29: static_assert(DCHECK_IS_ON() == ::logging::DEBUG_MODE, This is not simpler than #if DCHECK_IS_ON(), can you just use that? https://codereview.chromium.org/1701343003/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:95: CHECK(background_thread_pool_); why not DCHECK?
https://codereview.chromium.org/1701343003/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:87: ThreadPriority::BACKGROUND, 1U, IORestriction::DISALLOWED, On 2016/04/29 20:33:18, gab wrote: > On 2016/04/29 19:42:42, fdoray wrote: > > On 2016/04/29 18:34:22, robliao wrote: > > > Nit: More folks seem to use the lower case version for the unsigned > constant. > > 1U > > > -> 1u > > > > Done. > > Really? I've only ever since the upper-case one. > > Seems to be 2500 to 500 in favor of upper-case: > https://code.google.com/p/chromium/codesearch#search/&q=case:yes%20%5Cb%5B0-9... Done. https://codereview.chromium.org/1701343003/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/1701343003/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_scheduler.cc:8: #include "base/memory/ptr_util.h" On 2016/04/29 20:40:11, gab wrote: > unused Done. https://codereview.chromium.org/1701343003/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/1701343003/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:29: static_assert(DCHECK_IS_ON() == ::logging::DEBUG_MODE, On 2016/05/06 02:00:48, danakj wrote: > This is not simpler than #if DCHECK_IS_ON(), can you just use that? Done. https://codereview.chromium.org/1701343003/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:95: CHECK(background_thread_pool_); On 2016/05/06 02:00:48, danakj wrote: > why not DCHECK? We want to know if creating a thread pool can fail in production.
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/1701343003/#ps250001 (title: "rebase + CR danakj/gab")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1701343003/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1701343003/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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/1701343003/#ps270001 (title: "fix linux build error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1701343003/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1701343003/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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/1701343003/#ps290001 (title: "fix ios build error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1701343003/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1701343003/290001
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [13] TaskSchedulerImpl TaskSchedulerImpl is the default implementation of the TaskScheduler interface. It runs tasks in 4 thread pools: - Background with file i/o - Background without file i/o - Normal with file i/o - Normal without file i/o This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [13] TaskSchedulerImpl TaskSchedulerImpl is the default implementation of the TaskScheduler interface. It runs tasks in 4 thread pools: - Background with file i/o - Background without file i/o - Normal with file i/o - Normal without file i/o This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:290001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [13] TaskSchedulerImpl TaskSchedulerImpl is the default implementation of the TaskScheduler interface. It runs tasks in 4 thread pools: - Background with file i/o - Background without file i/o - Normal with file i/o - Normal without file i/o This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [13] TaskSchedulerImpl TaskSchedulerImpl is the default implementation of the TaskScheduler interface. It runs tasks in 4 thread pools: - Background with file i/o - Background without file i/o - Normal with file i/o - Normal without file i/o This change completes the necessary changes described in https://codereview.chromium.org/1698183005/ BUG=553459 Committed: https://crrev.com/4151ceaeb5627ddb2fd8c236c8fe055355c4c9a0 Cr-Commit-Position: refs/heads/master@{#392063} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/4151ceaeb5627ddb2fd8c236c8fe055355c4c9a0 Cr-Commit-Position: refs/heads/master@{#392063} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
