Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1708773002:
TaskScheduler [7] SchedulerThreadPool (Closed)
DescriptionTaskScheduler [7] SchedulerThreadPool
A SchedulerThreadPool manages a pool of threads that run Tasks. It
provides a CreateTaskRunnerWithTraits() method to create TaskRunners
whose PostTask invocations will result in scheduling Tasks on these
threads.
For now, only the PARALLEL ExecutionMode is supported.
This change is a subset of https://codereview.chromium.org/1698183005/
BUG=553459
Committed: https://crrev.com/7422e74e0ca8ff3bada133fce426c59a8094040c
Cr-Commit-Position: refs/heads/master@{#387593}
Patch Set 1 #Patch Set 2 : initial implementation for review #Patch Set 3 : self review #Patch Set 4 : self review #
Total comments: 30
Patch Set 5 : CR robliao #10 #Patch Set 6 : CR robliao #12 #Patch Set 7 : Use std::unique_ptr, rename ThreadPool -> SchedulerThreadPool. #
Total comments: 8
Patch Set 8 : CR robliao #15 #Patch Set 9 : TaskFactory in unit tests. #Patch Set 10 : format #Patch Set 11 : TaskFactory::PostTask #Patch Set 12 : self review #Patch Set 13 : self review #Patch Set 14 : self review #Patch Set 15 : self review #Patch Set 16 : self review #Patch Set 17 : self review #Patch Set 18 : self review #
Total comments: 24
Patch Set 19 : CR danakj #19 #Patch Set 20 : self review #Patch Set 21 : self review #Patch Set 22 : merge SchedulerWorkerThreadDelegate #
Total comments: 55
Patch Set 23 : merge SchedulerWorkerThread::Delegate #Patch Set 24 : move PostTaskHelper to its own file #Patch Set 25 : self review #
Total comments: 17
Patch Set 26 : CR gab/danakj #21-22 #Patch Set 27 : self review #Patch Set 28 : add SchedulerWorkerThreadDelegateImpl #
Total comments: 10
Patch Set 29 : CR gab/danakj #25-26 #
Total comments: 2
Patch Set 30 : CR gab #28 (rename lock) #Patch Set 31 : Wake up a thread explicitly when a Sequence is added in a PQ. #
Total comments: 31
Patch Set 32 : CR robliao #35 #
Total comments: 15
Patch Set 33 : CR gab #37-40 #Patch Set 34 : self review #
Total comments: 11
Patch Set 35 : CR danakj #43 #Patch Set 36 : remove static_assert #Patch Set 37 : fix windows build error #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 62 (20 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
Can you review this CL? Thanks.
Description was changed from ========== TaskScheduler [7/8] ThreadPool This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [7/8] ThreadPool This change is a subset of https://codereview.chromium.org/1698183005/ As documented in https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrk... BUG=553459 ==========
Description was changed from ========== TaskScheduler [7/8] ThreadPool This change is a subset of https://codereview.chromium.org/1698183005/ As documented in https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrk... BUG=553459 ========== to ========== TaskScheduler [7/8] ThreadPool This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Description was changed from ========== TaskScheduler [7/8] ThreadPool This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [8/9] ThreadPool This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Description was changed from ========== TaskScheduler [8/9] ThreadPool This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [7] ThreadPool This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Description was changed from ========== TaskScheduler [7] ThreadPool This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [7] ThreadPool A ThreadPool manages a pool threads that can run Tasks. It provides a CreateTaskRunnerWithTraits() method to create TaskRunners whose PostTask invocations will result in scheduling Tasks on its threads. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Description was changed from ========== TaskScheduler [7] ThreadPool A ThreadPool manages a pool threads that can run Tasks. It provides a CreateTaskRunnerWithTraits() method to create TaskRunners whose PostTask invocations will result in scheduling Tasks on its threads. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [7] ThreadPool A ThreadPool manages a pool threads that can run Tasks. It provides a CreateTaskRunnerWithTraits() method to create TaskRunners whose PostTask invocations will result in scheduling Tasks on these threads. For now, only the PARALLEL ExecutionMode is supported. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
robliao@: Can you review this CL? Thanks.
A first pass https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... File base/task_scheduler/thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:25: void PostTaskCallback(scoped_refptr<Sequence> sequence, Wasn't this and the below going to go into utils.h|cc? https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:27: scoped_ptr<Task> task) { Should DCHECK(task) as it's not immediately dereferenced. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:41: scoped_refptr<Sequence> sequence, sequence, priority_queue, and task should be DCHECKed as they aren't immediately dereferenced. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:102: if (thread_pool->worker_threads_.empty()) Does checking the thread count == num_threads make sense to do here? https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:131: // wake up another SchedulerWorkerThread to do so. Update this comment with the delegated GetWork. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:151: { Is this scope necessary? https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:188: if (worker_thread) { If we decide to fail when we couldn't create all the threads, we can fail-fast here. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:201: idle_worker_threads_stack_.top()->WakeUp(); Optional: It might be more robust to wake up the thread after we have popped it off of the idle worker threads stack. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:234: // |idle_worker_threads_stack_| to avoid this scenario: Nit: scenario -> race https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:238: // |shared_priority_queue_| and ends the Transaction. This couldn't Nit: couldn't -> can't https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... File base/task_scheduler/thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.h:59: // after |worker_thread| has run a Task from it. Note that |worker_thread| The second part of this is an implementation detail. Let's remove it. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... File base/task_scheduler/thread_pool_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool_unittest.cc:235: } Would it be useful to have a test that saturates the threadpool with a long running task (e.g. event wait) to verify we're able to wake up all the threads?
robliao@: Can you take another look? Thanks. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... File base/task_scheduler/thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:25: void PostTaskCallback(scoped_refptr<Sequence> sequence, On 2016/03/31 22:48:56, robliao wrote: > Wasn't this and the below going to go into utils.h|cc? Yes, it was previously in utils.h|.cc because it was used by SchedulerSingleThreadTaskRunner defined in worker_thread.cc and by Scheduler(Sequenced|Parallel)TaskRunner defined in thread_pool.cc. Now that a WorkerThread doesn't own its single-threaded PriorityQueue, all TaskRunners will be defined in thread_pool.cc so I think it makes sense to define these functions here. I moved PostTaskHelper outside of the anonymous namespace to allow testing. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:27: scoped_ptr<Task> task) { On 2016/03/31 22:48:55, robliao wrote: > Should DCHECK(task) as it's not immediately dereferenced. Done. DCHECK everything to be consistent. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:41: scoped_refptr<Sequence> sequence, On 2016/03/31 22:48:55, robliao wrote: > sequence, priority_queue, and task should be DCHECKed as they aren't immediately > dereferenced. Done. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:102: if (thread_pool->worker_threads_.empty()) On 2016/03/31 22:48:56, robliao wrote: > Does checking the thread count == num_threads make sense to do here? gab@ said that we should return a ThreadPool w/o error if we successfully created 1 thread. Do you think that an histogram telling us how often we fail to create the requested number of threads would be useful? https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:131: // wake up another SchedulerWorkerThread to do so. On 2016/03/31 22:48:56, robliao wrote: > Update this comment with the delegated GetWork. Done. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:151: { On 2016/03/31 22:48:55, robliao wrote: > Is this scope necessary? No. Removed it. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:188: if (worker_thread) { On 2016/03/31 22:48:55, robliao wrote: > If we decide to fail when we couldn't create all the threads, we can fail-fast > here. I think it makes sense to fail-fast because it is likely that creating a new thread will fail if the previous attempt failed. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:201: idle_worker_threads_stack_.top()->WakeUp(); On 2016/03/31 22:48:56, robliao wrote: > Optional: It might be more robust to wake up the thread after we have popped it > off of the idle worker threads stack. Done. I don't like the fact that it requires one more line of code. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:234: // |idle_worker_threads_stack_| to avoid this scenario: On 2016/03/31 22:48:55, robliao wrote: > Nit: scenario -> race Done. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:238: // |shared_priority_queue_| and ends the Transaction. This couldn't On 2016/03/31 22:48:55, robliao wrote: > Nit: couldn't -> can't Done. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... File base/task_scheduler/thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.h:59: // after |worker_thread| has run a Task from it. Note that |worker_thread| On 2016/03/31 22:48:56, robliao wrote: > The second part of this is an implementation detail. Let's remove it. Done. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... File base/task_scheduler/thread_pool_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool_unittest.cc:235: } On 2016/03/31 22:48:56, robliao wrote: > Would it be useful to have a test that saturates the threadpool with a long > running task (e.g. event wait) to verify we're able to wake up all the threads? Done. TaskSchedulerThreadPoolTest.Saturate
https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... File base/task_scheduler/thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:25: void PostTaskCallback(scoped_refptr<Sequence> sequence, On 2016/04/01 16:02:51, fdoray wrote: > On 2016/03/31 22:48:56, robliao wrote: > > Wasn't this and the below going to go into utils.h|cc? > > Yes, it was previously in utils.h|.cc because it was used by > SchedulerSingleThreadTaskRunner defined in worker_thread.cc and by > Scheduler(Sequenced|Parallel)TaskRunner defined in thread_pool.cc. Now that a > WorkerThread doesn't own its single-threaded PriorityQueue, all TaskRunners will > be defined in thread_pool.cc so I think it makes sense to define these functions > here. > > I moved PostTaskHelper outside of the anonymous namespace to allow testing. Given that PostTaskHelper is somewhat like a private function in this respect (no one should be directly calling it outside this file), it might make more sense to unit test the public callers of PostTaskHelper rather than PostTaskHelper directly. It's only one statement, after all. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:102: if (thread_pool->worker_threads_.empty()) On 2016/04/01 16:02:51, fdoray wrote: > On 2016/03/31 22:48:56, robliao wrote: > > Does checking the thread count == num_threads make sense to do here? > > gab@ said that we should return a ThreadPool w/o error if we successfully > created 1 thread. Do you think that an histogram telling us how often we fail to > create the requested number of threads would be useful? A histogram would make sense if we were able to take action on it. If we requested a threadpool of 4 threads and we ended up with 1 with no ability to grow, that could be surprising from a performance standpoint. I can see this make sense if we dynamically ramp up and down the number of threads in a threadpool and |num_threads| were simply an indication of the maximum number of threads allowed. That seems to make more sense at the moment. So num_threads -> max_threads documenting that there isn't a guarantee that you'll always get max_threads. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... File base/task_scheduler/thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.h:59: // after |worker_thread| has run a Task from it. Note that |worker_thread| On 2016/04/01 16:02:52, fdoray wrote: > On 2016/03/31 22:48:56, robliao wrote: > > The second part of this is an implementation detail. Let's remove it. > > Done. Looking at the code side-by-side, do we need worker_thread now that we check the current thread's shared priority queue? It's not read in the code..
robliao@: Can you take another look? Thanks. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... File base/task_scheduler/thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:25: void PostTaskCallback(scoped_refptr<Sequence> sequence, On 2016/04/01 19:14:49, robliao wrote: > On 2016/04/01 16:02:51, fdoray wrote: > > On 2016/03/31 22:48:56, robliao wrote: > > > Wasn't this and the below going to go into utils.h|cc? > > > > Yes, it was previously in utils.h|.cc because it was used by > > SchedulerSingleThreadTaskRunner defined in worker_thread.cc and by > > Scheduler(Sequenced|Parallel)TaskRunner defined in thread_pool.cc. Now that a > > WorkerThread doesn't own its single-threaded PriorityQueue, all TaskRunners > will > > be defined in thread_pool.cc so I think it makes sense to define these > functions > > here. > > > > I moved PostTaskHelper outside of the anonymous namespace to allow testing. > > Given that PostTaskHelper is somewhat like a private function in this respect > (no one should be directly calling it outside this file), it might make more > sense to unit test the public callers of PostTaskHelper rather than > PostTaskHelper directly. It's only one statement, after all. PostTaskHelper is only one statement, but it indirectly calls PostTaskCallback which implements the "post task" algorithm that we described in the design doc. I would like to keep a unit test for this algorithm. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.cc:102: if (thread_pool->worker_threads_.empty()) On 2016/04/01 19:14:49, robliao wrote: > On 2016/04/01 16:02:51, fdoray wrote: > > On 2016/03/31 22:48:56, robliao wrote: > > > Does checking the thread count == num_threads make sense to do here? > > > > gab@ said that we should return a ThreadPool w/o error if we successfully > > created 1 thread. Do you think that an histogram telling us how often we fail > to > > create the requested number of threads would be useful? > > A histogram would make sense if we were able to take action on it. > > If we requested a threadpool of 4 threads and we ended up with 1 with no ability > to grow, that could be surprising from a performance standpoint. > > I can see this make sense if we dynamically ramp up and down the number of > threads in a threadpool and |num_threads| were simply an indication of the > maximum number of threads allowed. That seems to make more sense at the moment. > > So num_threads -> max_threads documenting that there isn't a guarantee that > you'll always get max_threads. Done. Renamed num_threads -> max_threads. I think it's better to return a ThreadPool with less than |max_threads| than to crash when we aren't able to create all requested threads. I would like to work on dynamic ramp up/down once we have an initial implementation of the task scheduler in base/. I think it's better to wait until we have this before we add an histogram for thread creation failure. https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... File base/task_scheduler/thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/60001/base/task_scheduler/thr... base/task_scheduler/thread_pool.h:59: // after |worker_thread| has run a Task from it. Note that |worker_thread| On 2016/04/01 19:14:49, robliao wrote: > On 2016/04/01 16:02:52, fdoray wrote: > > On 2016/03/31 22:48:56, robliao wrote: > > > The second part of this is an implementation detail. Let's remove it. > > > > Done. > > Looking at the code side-by-side, do we need worker_thread now that we check the > current thread's shared priority queue? It's not read in the code.. Removed it.
Description was changed from ========== TaskScheduler [7] ThreadPool A ThreadPool manages a pool threads that can run Tasks. It provides a CreateTaskRunnerWithTraits() method to create TaskRunners whose PostTask invocations will result in scheduling Tasks on these threads. For now, only the PARALLEL ExecutionMode is supported. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [7] SchedulerThreadPool A SchedulerThreadPool manages a pool threads that run Tasks. It provides a CreateTaskRunnerWithTraits() method to create TaskRunners whose PostTask invocations will result in scheduling Tasks on these threads. For now, only the PARALLEL ExecutionMode is supported. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
lgtm + nits. https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:124: // If |worker_thread| belongs to this thread pool, set a flag to avoid waking Update this comment. https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:60: // Reinserts |sequence| in |shared_priority_queue_| with |sequence_sort_key| Nit: Into this thread's shared priority queue. (My recollection from a previous CL is that we're avoiding referencing private vars here, correct)? https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:61: // after one of its Tasks ran. The "after one of its Tasks ran" can be removed. It's an implementation detail of when we could call this. https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:217: true, // Manual reset, to wake up multiple waiters at once. Nit: I think we can omit these comments. The premise being that readers are considered to be "experts" in this code. It's unfortunate, but this is why I prefer enums to booleans for these cases :-) .
fdoray@chromium.org changed reviewers: + danakj@chromium.org
danakj@: Can you review this CL? Thanks. robliao@: All done. Thanks for the reviews. https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:124: // If |worker_thread| belongs to this thread pool, set a flag to avoid waking On 2016/04/01 21:05:51, robliao wrote: > Update this comment. Done. https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:60: // Reinserts |sequence| in |shared_priority_queue_| with |sequence_sort_key| On 2016/04/01 21:05:51, robliao wrote: > Nit: Into this thread's shared priority queue. > (My recollection from a previous CL is that we're avoiding referencing private > vars here, correct)? Done. https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:61: // after one of its Tasks ran. On 2016/04/01 21:05:51, robliao wrote: > The "after one of its Tasks ran" can be removed. It's an implementation detail > of when we could call this. Done. https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:217: true, // Manual reset, to wake up multiple waiters at once. On 2016/04/01 21:05:51, robliao wrote: > Nit: I think we can omit these comments. The premise being that readers are > considered to be "experts" in this code. > > It's unfortunate, but this is why I prefer enums to booleans for these cases :-) > . Done.
Description was changed from ========== TaskScheduler [7] SchedulerThreadPool A SchedulerThreadPool manages a pool threads that run Tasks. It provides a CreateTaskRunnerWithTraits() method to create TaskRunners whose PostTask invocations will result in scheduling Tasks on these threads. For now, only the PARALLEL ExecutionMode is supported. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [7] SchedulerThreadPool A SchedulerThreadPool manages a pool of threads that run Tasks. It provides a CreateTaskRunnerWithTraits() method to create TaskRunners whose PostTask invocations will result in scheduling Tasks on these threads. For now, only the PARALLEL ExecutionMode is supported. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:41: DCHECK(delay.is_zero()); Can you leave a comment in this method explaining how the implementation here related to PARALLEL execution. I think it's because it always makes a new sequence? If true, that fact is a bit subtle. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:42: PostTaskHelper(WrapUnique(new Task(from_here, closure, traits_)), There's so much indirection here, I actually had to draw diagrams on paper to understand the flow, which is usually a bad sign. In the PostTaskHelper, I think that we end up with: - A callback to PostTaskCallback binding a sequence (owned) and queue (unowned) - Being passed to a task tracker with a Task which holds a closure - That runs the first callback with the Task Have you thought about ways to make this simpler? The "passed to a task tracker" requires looking over there to recall that it's simply running the callback. The "PostTask" name made me assume it's going to bind those things into yet another callback and post that somewhere.. maybe we could make that method name better now that we see use of it. Or maybe PostTask should just take a Task* and return bool if you should post it, because there's no reason it has to do the posting, we're passing the function to do that posting to it. Conceptually I think what this code is doing is.. If (task_tracker->WillPostTask(task) && sequence->PushTask(move(task))) { queue->BeginTransation()->Push(sequence) } If you are super concerned about compiler-enforcing that task_tracker is called before PushTask (are you?), we could have task hold some DCHECK bool to verify, or have it return some testable non-bool type and pass that to PushTask, or something. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:69: if (sequence->PushTask(std::move(task))) { Can you use a temp var here to document what the PushTask return value was rather than the comment below it? The code reads here like success/fail until you get to the comment, and then you're not quite sure how they relate. So something like bool sequence_was_empty = sequence->PushTask(task) if (sequence_was_empty) { ... } https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:111: NOTIMPLEMENTED(); nit, but do you want to be writing code that checks this method's return for null? I'm assuming not, in which case NOTREACHED() makes more sense probably. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:124: // If the current thread belongs to this thread pool, set a flag to avoid Why do we do this on reinsert but not on the initial insert? The implication is that we're running a task from |sequence| already if we get here and the current thread is running the priority queue? And that we would not want to use another thread to run the next task in the sequence? And that there won't be some other sequence next in the priority queue? Probably some of the above are not true, curious which. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:132: shared_priority_queue_.BeginTransaction()->Push( Hrm, can you explain why we need the whole SequenceInsertedInSharedPriorityQueueCallback thing? Why can't we just call WakeUpOneThread() from here after we insert the sequence? (I feel like code patterns that go from class A -> class B -> class A in a single stack are a somewhat anti-pattern, which this creates here.) https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:166: DCHECK_GT(max_threads, 0U); This is a pretty complicated constructor. Style guide says we should break logic out into an Initialize method. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:213: if (!no_wake_up_on_sequence_insertion_.Get()) I'm not a fan of "no" in bool names, the amount of negating gets hard to read. Especially now that you're using !no here. Can you invert this boolean, or word it inversely without using negatives in it? https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:236: // if the Transaction of step 1 is still active. Mention that the shared_priority_queue has no predecessor, so there's no other queue that could have a Transaction active? https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:60: // Reinserts |sequence| into this thread pool's shared priority queue with Can you explain in the comment why this method is "Reinsert" instead of just "Insert", like it implies things are already inserted somewhere, so when would this be called? https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:83: void AddToIdleSchedulerWorkerThreadsStack( Quite the mouthful. Maybe just AddToIdleWorkerThreadsStack? https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:61: if (!sequence->PopTask()) { document what this return value means with a temp var, sequence_became_empty or something.
danakj@: Can you take another look? This CL now depends on these 2 small CLs: - https://codereview.chromium.org/1868673002/ - https://codereview.chromium.org/1862113002/ Thanks! https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:41: DCHECK(delay.is_zero()); On 2016/04/05 00:27:59, danakj wrote: > Can you leave a comment in this method explaining how the implementation here > related to PARALLEL execution. I think it's because it always makes a new > sequence? If true, that fact is a bit subtle. Done. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:42: PostTaskHelper(WrapUnique(new Task(from_here, closure, traits_)), On 2016/04/05 00:27:59, danakj wrote: > There's so much indirection here, I actually had to draw diagrams on paper to > understand the flow, which is usually a bad sign. > > In the PostTaskHelper, I think that we end up with: > > - A callback to PostTaskCallback binding a sequence (owned) and queue (unowned) > - Being passed to a task tracker with a Task which holds a closure > - That runs the first callback with the Task > > Have you thought about ways to make this simpler? > > The "passed to a task tracker" requires looking over there to recall that it's > simply running the callback. The "PostTask" name made me assume it's going to > bind those things into yet another callback and post that somewhere.. maybe we > could make that method name better now that we see use of it. Or maybe PostTask > should just take a Task* and return bool if you should post it, because there's > no reason it has to do the posting, we're passing the function to do that > posting to it. > > Conceptually I think what this code is doing is.. > > If (task_tracker->WillPostTask(task) && sequence->PushTask(move(task))) { > queue->BeginTransation()->Push(sequence) > } > > If you are super concerned about compiler-enforcing that task_tracker is called > before PushTask (are you?), we could have task hold some DCHECK bool to verify, > or have it return some testable non-bool type and pass that to PushTask, or > something. You understand correctly what this code is doing. I changed "void TaskTracker::PostTask(std::unique_ptr<Task>, PostTaskCallback)" to "bool TaskTracker::WillPostTask(const Task*)" to remove one level of indirection https://codereview.chromium.org/1862113002/ I agree that it makes the code simpler. However, we liked the fact that with a callback, TaskTracker knew for sure that the task would be posted after it updated |num_tasks_blocking_shutdown_| (now it needs to rely of the caller to post the task). https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:69: if (sequence->PushTask(std::move(task))) { On 2016/04/05 00:27:59, danakj wrote: > Can you use a temp var here to document what the PushTask return value was > rather than the comment below it? The code reads here like success/fail until > you get to the comment, and then you're not quite sure how they relate. So > something like > > bool sequence_was_empty = sequence->PushTask(task) > if (sequence_was_empty) { > ... > } Done. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:111: NOTIMPLEMENTED(); On 2016/04/05 00:27:59, danakj wrote: > nit, but do you want to be writing code that checks this method's return for > null? I'm assuming not, in which case NOTREACHED() makes more sense probably. Done. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:124: // If the current thread belongs to this thread pool, set a flag to avoid On 2016/04/05 00:27:59, danakj wrote: > Why do we do this on reinsert but not on the initial insert? The implication is > that we're running a task from |sequence| already if we get here and the current > thread is running the priority queue? > > And that we would not want to use another thread to run the next task in the > sequence? > > And that there won't be some other sequence next in the priority queue? > > Probably some of the above are not true, curious which. I tried to clarify the comment. 1. "The implication is that we're running a task from |sequence| already if we get here": Yes. In the .h file, I added that this method must only be called by worker thread after running a task from |sequence|. 2. "and the current thread is running the priority queue?": Not necessarily. "g_current_shared_priority_queue.Get().Get() == &shared_priority_queue_" tells us whether that's the case. A high priority thread can use this method to insert a Sequence in the shared priority queue of a low priority thread pool (happens when the high priority thread runs the last high priority Task in a Sequence that also contains low priority Tasks). 3. "And that we would not want to use another thread to run the next task in the sequence?" That's not the goal. If the current thread belongs to this thread pool, it will soon pop a sequence from |shared_priority_queue_| via GetWork(). Waking up another thread in that situation would simply be a waste of resources (2 threads are gonna invoke GetWork() after we've added 1 Sequence into |shared_priority_queue_|). 4. "And that there won't be some other sequence next in the priority queue?" There might be other sequences in |shared_priority_queue_|. However, if the current thread doesn't get |sequence| the next time it calls GetWork(), another thread will get it eventually (could be the thread that was woken up when the Sequence that this thread got was inserted in |shared_priority_queue_|. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:132: shared_priority_queue_.BeginTransaction()->Push( On 2016/04/05 00:27:59, danakj wrote: > Hrm, can you explain why we need the whole > SequenceInsertedInSharedPriorityQueueCallback thing? Why can't we just call > WakeUpOneThread() from here after we insert the sequence? > > (I feel like code patterns that go from class A -> class B -> class A in a > single stack are a somewhat anti-pattern, which this creates here.) After a Sequence is inserted into a PriorityQueue [1], we usually wake up a thread [2]. With a "sequence inserted" callback, those who insert Sequences into PriorityQueues don't have to know how to perform this wake up. This will be very helpful once we start having different wake up mechanism for different PriorityQueues (ThreadPool::WakeUpOneThread for a shared priority queue, WorkerThread::WakeUp for a single-threaded PriorityQueue). [1] A Sequence can be inserted into a PriorityQueue in 3 situations: - After a Task is added in the Sequence by a TaskRunner. - After a Task is added in the Sequence by the delayed tasks manager (when the Task's delay expires). - After a worker thread ran a Task from the Sequence. [2] See discussion in SchedulerThreadPool::ReinsertSequence for an exception to this rule. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:166: DCHECK_GT(max_threads, 0U); On 2016/04/05 00:27:59, danakj wrote: > This is a pretty complicated constructor. Style guide says we should break logic > out into an Initialize method. > Done. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:213: if (!no_wake_up_on_sequence_insertion_.Get()) On 2016/04/05 00:27:59, danakj wrote: > I'm not a fan of "no" in bool names, the amount of negating gets hard to read. > Especially now that you're using !no here. Can you invert this boolean, or word > it inversely without using negatives in it? I removed this variable and added a PriorityQueue::Transaction::PushNoWakeUp() method instead https://codereview.chromium.org/1868673002/. We decided that less state is better. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:236: // if the Transaction of step 1 is still active. On 2016/04/05 00:27:58, danakj wrote: > Mention that the shared_priority_queue has no predecessor, so there's no other > queue that could have a Transaction active? Not sure why this would be relevant here. Tried to make the text less ambiguous by adding "because there can only be one active Transaction per PriorityQueue at a time". https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:60: // Reinserts |sequence| into this thread pool's shared priority queue with On 2016/04/05 00:27:59, danakj wrote: > Can you explain in the comment why this method is "Reinsert" instead of just > "Insert", like it implies things are already inserted somewhere, so when would > this be called? Renamed ReinsertSequence -> InsertSequenceAfterTaskRan. This method must be called from a worker thread that just ran a Task from |sequence|. That means that |sequence| has already been in a PriorityQueue in the past, hence the use of "Reinsert" in the previous patch set. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:83: void AddToIdleSchedulerWorkerThreadsStack( On 2016/04/05 00:27:59, danakj wrote: > Quite the mouthful. Maybe just AddToIdleWorkerThreadsStack? Done. https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/330001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:61: if (!sequence->PopTask()) { On 2016/04/05 00:27:59, danakj wrote: > document what this return value means with a temp var, sequence_became_empty or > something. Done.
Another great component :-), thanks for keeping SEQUENCED/SINGLE_THREAD/dynamic thread creation for future CLs this one was just the right amount of complexity. Cheers, Gab https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:44: // Create a new Sequence to allow parallel execution of Tasks posted through s/new Sequence/single-task Sequence/ ("create" implies "new", but "single-task" is what's important to highlight here) https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:46: scoped_refptr<Sequence> sequence(new Sequence); I'd also be happy to inline this, something like: // Post the task as part of a one-off single-task Sequence. PostTaskHelper(WrapUnique(new Task(from_here, closure, traits_)), make_scoped_refptr(new Sequence), priority_queue_, task_tracker_); https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:85: DCHECK(join_for_testing_returned_ || worker_threads_.empty()); Wrap these two lines in #if DCHECK_IS_ON() to avoid locking in Release builds? (or even compiling it in given it's never supposed to run) Also add a comment verbally explaining this check like: // SchedulerThreadPool should never be deleted in production unless its initialization failed. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:138: AutoSchedulerLock auto_lock(join_for_testing_returned_lock_); We could get away with a MemoryBarrier instead of a lock? Or shall we use a WaitableEvent? Essentially this is a boolean whose state needs to be synced across threads. WaitableEvent sounds fine to me (I'd like to reduce the number of members which are test only). https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:181: worker_thread->WakeUp(); Could technically release the lock before invoking |WakeUp()| if we think this may help performance. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:191: idle_worker_threads_stack_cv_->Broadcast(); Would a manual-reset WaitableEvent be better here? It feels weird to make a broadcast of something that might become false any second (I understand it probably doesn't in the controlled way the test is set up but the reader of this code doesn't know that). https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:201: tls_current_shared_priority_queue.Get().Set(nullptr); Actually looking into this deeper turns out this is not required. This line merely sets the mapping to null instead of an invalid pointer it doesn't actually remove the slot allocated for this thread. But this automatically does https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr.... In SchedulerLock we actually had to use ThreadLocalStorage::Slot directly instead of ThreadLocalPointer per the pointer being put in there being owned by the TLS state and us thus needing to register custom destructor handling. In this case it's not owned and letting the map auto clear itself on thread exit is sufficient. (ThreadLocalPointer is implemented in terms of ThreadLocalStorage::StaticSlot on Windows anyways -- Rob can rant some more here as to why we still have the two very similar impls, mostly an unfinished cleanup FWIR) https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:228: transaction->Pop(); Actually since this is all done under a single transaction could we just have Pop() return the value and remove Peek()? https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:256: const SequenceSortKey sequence_sort_key = sequence->GetSortKey(); inline this? https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:45: // to create a thread pool with at least one thread. s/when it's not possible to/on failure to/ https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:58: // Tasks with |traits| and |execution_mode| in this thread pool. Does anything make sure that this STP is the right pool for these |traits| or is that up to the caller to decide? https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:64: // |sequence_sort_key|. Must only be used by a worker thread to put |sequence| I would assume SWT doesn't have a pointer back to its STP and that this is instead bound into a callback and that this can thus be priavte (actually isn't this RanTaskFromSequence() -- I'm confused on the difference here, is it because half the decision is made outside and loops back into InsertSequenceAfterTaskRan() if it should be re-inserted in this ThreadPool?) https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:83: // Wakes up one thread from this thread pool if they aren't all busy. How about: // Wakes up the last thread from this thread pool to go idle, if any. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:100: // constructor. I don't think this last requirement "This is only modified by the constructor" is necessary. I definitely see a future world where the list of worker_threads_ will be dynamic. Doesn't need to be now but no need to add this API explicit requirement and restrict future evolutions of the impl. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:104: // |idle_worker_threads_cv_|. Add a comment explaining why it takes the shared PQ lock as a predecessor. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:108: // top of the stack. rm "The last worker thread that became idle is top of the stack." IMO this is implicit in the definition of a stack (where it's worth highlighting IMO is on the APIs above as requested there). https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:112: std::unique_ptr<ConditionVariable> idle_worker_threads_stack_cv_; Should we brand this with "for_testing"? I was confused why this was required until I realized it was test-only. We should at least have its comment mention this? https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:61: if (!sequence_became_empty) { As discussed offline, make the callback EnqueueSequence() instead and have the WorkerThread do the pop, only calling this callback if it actually needs to enqueue, centralizing the pop logic in WorkerThread. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:108: PostTask(task_runner, false, nullptr); I was curious what this top-level PostTask() call was until I realized it was the factory's, how about PostTestTask() to make it clear that callers are calling something in this test namespace and not one of our PostTask APIs. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:117: run_tasks_.insert(task_index); s/run_tasks_/ran_tasks_/ https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:157: post_nested_task_(post_nested_task) {} DCHECK(thread_pool_) https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:173: scoped_refptr<TaskRunner> task_runner_; "const scoped_refptr" as well? https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:297: TEST(TaskSchedulerPostTaskHelperTest, PostTaskInEmptySequence) { Don't use the fixture name (TaskSchedulerPostTaskHelperTest) for tests that aren't TEST_F
https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:112: std::unique_ptr<ConditionVariable> idle_worker_threads_stack_cv_; On 2016/04/07 20:32:48, gab wrote: > Should we brand this with "for_testing"? I was confused why this was required > until I realized it was test-only. +1 https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:206: const PriorityQueue::SequenceAndSortKey sequence_and_sort_key( nit: auto? https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:34: class BASE_EXPORT SchedulerThreadPool : public SchedulerWorkerThread::Delegate { nit: maybe consider putting the delegate in its own header file and not nesting it (avoids this header needing to know about the whole SchedulerWorkerThread). I've been frustrated over time with public nested types in general, so just a feeling it might help you one day. feel free to leave it too. https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:88: // SchedulerWorkerThread::Delegate: nit: not really sure why these are private, you can upcast and call them anyways. and they are called from outside this class as the delegate, so it's a bit misleading maybe. https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/ut... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/ut... base/task_scheduler/utils.cc:18: bool PostTaskHelper(std::unique_ptr<Task> task, Can this be a method on SchedulerThreadPool? That feels like it's advantageous in a few ways: - We don't leave scheduler_thread_pool.cc when we're posting tasks, easier to follow. - We don't have to pass around queue pointers and stuff, we just use the members. - That makes SchedulerThreadPool the only caller of BeginTransaction()->Push() right? Then maybe it can just call WakeUpOneThread on itself instead of going (SchedulerThreadPool or *TaskRunner) -> PriorityQueue::Transaction -> SchedulerThreadPool to wake up a thread.
https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:44: // Create a new Sequence to allow parallel execution of Tasks posted through On 2016/04/07 20:32:47, gab wrote: > s/new Sequence/single-task Sequence/ > > ("create" implies "new", but "single-task" is what's important to highlight > here) Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:46: scoped_refptr<Sequence> sequence(new Sequence); On 2016/04/07 20:32:47, gab wrote: > I'd also be happy to inline this, something like: > > // Post the task as part of a one-off single-task Sequence. > PostTaskHelper(WrapUnique(new Task(from_here, closure, traits_)), > make_scoped_refptr(new Sequence), priority_queue_, > task_tracker_); Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:85: DCHECK(join_for_testing_returned_ || worker_threads_.empty()); On 2016/04/07 20:32:47, gab wrote: > Wrap these two lines in #if DCHECK_IS_ON() to avoid locking in Release builds? > (or even compiling it in given it's never supposed to run) > > Also add a comment verbally explaining this check like: > > // SchedulerThreadPool should never be deleted in production unless its > initialization failed. Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:138: AutoSchedulerLock auto_lock(join_for_testing_returned_lock_); On 2016/04/07 20:32:47, gab wrote: > We could get away with a MemoryBarrier instead of a lock? > > Or shall we use a WaitableEvent? Essentially this is a boolean whose state needs > to be synced across threads. WaitableEvent sounds fine to me (I'd like to reduce > the number of members which are test only). I wanted to use a MemoryBarrier but https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... Let's use a WaitableEvent. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:181: worker_thread->WakeUp(); On 2016/04/07 20:32:47, gab wrote: > Could technically release the lock before invoking |WakeUp()| if we think this > may help performance. Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:191: idle_worker_threads_stack_cv_->Broadcast(); On 2016/04/07 20:32:47, gab wrote: > Would a manual-reset WaitableEvent be better here? It feels weird to make a > broadcast of something that might become false any second (I understand it > probably doesn't in the controlled way the test is set up but the reader of this > code doesn't know that). The WaitableEvent would be reset in WakeUpOneThread, when a thread is removed from the stack? I don't like this solution because it means that we'll have to think about resetting the event whenever we remove a thread from the stack (eventually, we'll also remove a thread from the stack when single-threaded work is posted). Currently, WaitForAllWorkerThreadsIdleForTesting only returns when all threads are idle. This state can change immediately after, but it would also be the case with a WaitableEvent. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:201: tls_current_shared_priority_queue.Get().Set(nullptr); On 2016/04/07 20:32:47, gab wrote: > Actually looking into this deeper turns out this is not required. > This line merely sets the mapping to null instead of an invalid pointer it > doesn't actually remove the slot allocated for this thread. > > But this automatically does > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr.... > > In SchedulerLock we actually had to use ThreadLocalStorage::Slot directly > instead of ThreadLocalPointer per the pointer being put in there being owned by > the TLS state and us thus needing to register custom destructor handling. In > this case it's not owned and letting the map auto clear itself on thread exit is > sufficient. > > (ThreadLocalPointer is implemented in terms of ThreadLocalStorage::StaticSlot on > Windows anyways -- Rob can rant some more here as to why we still have the two > very similar impls, mostly an unfinished cleanup FWIR) Done. Interesting. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:228: transaction->Pop(); On 2016/04/07 20:32:47, gab wrote: > Actually since this is all done under a single transaction could we just have > Pop() return the value and remove Peek()? Eventually, we'll have to: - peek from a shared and a single-threaded PQ - determine which sequence has the highest priority - pop it from its PQ We need separate methods for peek/pop to do that. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:256: const SequenceSortKey sequence_sort_key = sequence->GetSortKey(); On 2016/04/07 20:32:47, gab wrote: > inline this? I can't. I have to make sure that sequence->GetSortKey() runs before std::move(sequence). https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:45: // to create a thread pool with at least one thread. On 2016/04/07 20:32:48, gab wrote: > s/when it's not possible to/on failure to/ Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:58: // Tasks with |traits| and |execution_mode| in this thread pool. On 2016/04/07 20:32:48, gab wrote: > Does anything make sure that this STP is the right pool for these |traits| or is > that up to the caller to decide? It's up to the caller to decide. The mapping TaskTraits -> ThreadPool will live in TaskSchedulerImpl. If we add checks in ThreadPool, we'll have to update multiple components when we decide to change the mapping. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:64: // |sequence_sort_key|. Must only be used by a worker thread to put |sequence| On 2016/04/07 20:32:48, gab wrote: > I would assume SWT doesn't have a pointer back to its STP and that this is > instead bound into a callback and that this can thus be priavte (actually isn't > this RanTaskFromSequence() -- I'm confused on the difference here, is it because > half the decision is made outside and loops back into > InsertSequenceAfterTaskRan() if it should be re-inserted in this ThreadPool?) Currently, the stack to reinsert a Sequence in a shared PriorityQueue after a pop is: 1. SchedulerWorkerThread::ThreadMain 2. SchedulerThreadPool::EnqueueSequence (delegate) 3. TaskSchedulerImpl::EnqueueSequenceAfterTaskRan (enqueue_sequence_callback_) [1] 4. SchedulerThreadPool::EnqueueSequenceAfterTaskRan [2] [1] Computes the Sequence's sort key and determines in which PQ it should be reinserted. [2] The SchedulerThreadPool isn't necessarily the same as in 2. When WorkerThread had multiple callbacks instead of a delegate, 1 invoked 3 directly and we didn't have 2 methods with similar names on ThreadPool (the names are very bad currently). Can I remove EnqueueSequence from SchedulerWorkerThread::Delegate and use a callback to go directly from 1 to 3? https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:83: // Wakes up one thread from this thread pool if they aren't all busy. On 2016/04/07 20:32:47, gab wrote: > How about: > > // Wakes up the last thread from this thread pool to go idle, if any. Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:100: // constructor. On 2016/04/07 20:32:48, gab wrote: > I don't think this last requirement "This is only modified by the constructor" > is necessary. I definitely see a future world where the list of worker_threads_ > will be dynamic. Doesn't need to be now but no need to add this API explicit > requirement and restrict future evolutions of the impl. Done. Without this comment, I have to hold a lock to access |worker_threads_|. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:104: // |idle_worker_threads_cv_|. On 2016/04/07 20:32:48, gab wrote: > Add a comment explaining why it takes the shared PQ lock as a predecessor. Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:108: // top of the stack. On 2016/04/07 20:32:48, gab wrote: > rm "The last worker thread that became idle is top of the stack." > > IMO this is implicit in the definition of a stack (where it's worth highlighting > IMO is on the APIs above as requested there). Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:112: std::unique_ptr<ConditionVariable> idle_worker_threads_stack_cv_; On 2016/04/07 23:08:43, danakj wrote: > On 2016/04/07 20:32:48, gab wrote: > > Should we brand this with "for_testing"? I was confused why this was required > > until I realized it was test-only. > > +1 Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:61: if (!sequence_became_empty) { On 2016/04/07 20:32:48, gab wrote: > As discussed offline, make the callback EnqueueSequence() instead and have the > WorkerThread do the pop, only calling this callback if it actually needs to > enqueue, centralizing the pop logic in WorkerThread. Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:108: PostTask(task_runner, false, nullptr); On 2016/04/07 20:32:48, gab wrote: > I was curious what this top-level PostTask() call was until I realized it was > the factory's, how about PostTestTask() to make it clear that callers are > calling something in this test namespace and not one of our PostTask APIs. Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:117: run_tasks_.insert(task_index); On 2016/04/07 20:32:48, gab wrote: > s/run_tasks_/ran_tasks_/ Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:157: post_nested_task_(post_nested_task) {} On 2016/04/07 20:32:48, gab wrote: > DCHECK(thread_pool_) Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:173: scoped_refptr<TaskRunner> task_runner_; On 2016/04/07 20:32:48, gab wrote: > "const scoped_refptr" as well? Done. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:297: TEST(TaskSchedulerPostTaskHelperTest, PostTaskInEmptySequence) { On 2016/04/07 20:32:48, gab wrote: > Don't use the fixture name (TaskSchedulerPostTaskHelperTest) for tests that > aren't TEST_F Moved these tests to utils_unittest.cc. Not sure what your comment is supposed to mean. https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:206: const PriorityQueue::SequenceAndSortKey sequence_and_sort_key( On 2016/04/07 23:08:44, danakj wrote: > nit: auto? Done. https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:34: class BASE_EXPORT SchedulerThreadPool : public SchedulerWorkerThread::Delegate { On 2016/04/07 23:08:44, danakj wrote: > nit: maybe consider putting the delegate in its own header file and not nesting > it (avoids this header needing to know about the whole SchedulerWorkerThread). > I've been frustrated over time with public nested types in general, so just a > feeling it might help you one day. feel free to leave it too. gab@: You asked me to nest Delegate inside SchedulerWorkerThread. Do you mind if we move it back to its own .h file? https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:88: // SchedulerWorkerThread::Delegate: On 2016/04/07 23:08:44, danakj wrote: > nit: not really sure why these are private, you can upcast and call them > anyways. and they are called from outside this class as the delegate, so it's a > bit misleading maybe. I'd like to keep these methods private to document that they shouldn't shouldn't be called from a SchedulerThreadPool*. If you upcast to SchedulerWorkerThread::Delegate* outside of SchedulerWorkerThread, it's a hint that you are doing something wrong. Relevant discussion: https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/CwzjcWd9cYM... If you disagree with this, I'll make the methods public. https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/ut... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/ut... base/task_scheduler/utils.cc:18: bool PostTaskHelper(std::unique_ptr<Task> task, On 2016/04/07 23:08:44, danakj wrote: > Can this be a method on SchedulerThreadPool? > > That feels like it's advantageous in a few ways: > - We don't leave scheduler_thread_pool.cc when we're posting tasks, easier to > follow. > - We don't have to pass around queue pointers and stuff, we just use the > members. > - That makes SchedulerThreadPool the only caller of BeginTransaction()->Push() > right? Then maybe it can just call WakeUpOneThread on itself instead of going > (SchedulerThreadPool or *TaskRunner) -> PriorityQueue::Transaction -> > SchedulerThreadPool to wake up a thread. This method will also be used with single-threaded PriorityQueues. If I move it to SchedulerThreadPool: - |priority_queue| will still be an argument. - single-threaded task runners declared in scheduler_worker_thread.cc will have to know about SchedulerThreadPool.
danakj@/gab@: Can you take another look? Thanks. https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:88: // SchedulerWorkerThread::Delegate: On 2016/04/08 14:53:03, fdoray wrote: > On 2016/04/07 23:08:44, danakj wrote: > > nit: not really sure why these are private, you can upcast and call them > > anyways. and they are called from outside this class as the delegate, so it's > a > > bit misleading maybe. > > I'd like to keep these methods private to document that they shouldn't shouldn't > be called from a SchedulerThreadPool*. If you upcast to > SchedulerWorkerThread::Delegate* outside of SchedulerWorkerThread, it's a hint > that you are doing something wrong. Relevant discussion: > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/CwzjcWd9cYM... > > If you disagree with this, I'll make the methods public. Moved all these methods to SchedulerWorkerThreadDelegateImpl. This solves the visibility problem that you pointed out and avoids having two EnqueueSequence methods in SchedulerThreadPool (one that forwards to enqueue_sequence_callback_ and one that actually enqueues the sequences it receives).
Review of diff from 22=>28. https://codereview.chromium.org/1708773002/diff/410001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1708773002/diff/410001/base/base.gypi#newcode649 base/base.gypi:649: 'task_scheduler/scheduler_worker_thread_delegate.h', Careful with doing merges in their own PS, the diff between 28 and 22 includes something else and no PS indicate a merge. Looks like it's only affecting gyp in this diff, but I'm always leery when I see something unrelated that the rest of the diff I'm looking at is potentially including a merged CL... https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:191: idle_worker_threads_stack_cv_->Broadcast(); On 2016/04/08 14:53:02, fdoray wrote: > On 2016/04/07 20:32:47, gab wrote: > > Would a manual-reset WaitableEvent be better here? It feels weird to make a > > broadcast of something that might become false any second (I understand it > > probably doesn't in the controlled way the test is set up but the reader of > this > > code doesn't know that). > > The WaitableEvent would be reset in WakeUpOneThread, when a thread is removed > from the stack? I don't like this solution because it means that we'll have to > think about resetting the event whenever we remove a thread from the stack > (eventually, we'll also remove a thread from the stack when single-threaded work > is posted). Currently, WaitForAllWorkerThreadsIdleForTesting only returns when > all threads are idle. This state can change immediately after, but it would also > be the case with a WaitableEvent. But the Broadcast's call can racily be lost, no? i.e. if the broadcast occurs before the WaitForAllWorkerThreadsIdleForTesting call, the wait will block forever? https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:100: // constructor. On 2016/04/08 14:53:02, fdoray wrote: > On 2016/04/07 20:32:48, gab wrote: > > I don't think this last requirement "This is only modified by the constructor" > > is necessary. I definitely see a future world where the list of > worker_threads_ > > will be dynamic. Doesn't need to be now but no need to add this API explicit > > requirement and restrict future evolutions of the impl. > > Done. Without this comment, I have to hold a lock to access |worker_threads_|. Good point, but I think that's the right thing to do (we'll have to long term anyways). https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:297: TEST(TaskSchedulerPostTaskHelperTest, PostTaskInEmptySequence) { On 2016/04/08 14:53:03, fdoray wrote: > On 2016/04/07 20:32:48, gab wrote: > > Don't use the fixture name (TaskSchedulerPostTaskHelperTest) for tests that > > aren't TEST_F > > Moved these tests to utils_unittest.cc. Not sure what your comment is supposed > to mean. Oops, actually no I had somehow read "TaskSchedulerPostTaskHelperTest" as being "TaskSchedulerThreadPoolTest"... "TaskSchedulerPostTaskHelperTest" is another name and was correct, please do keep them here. https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:34: class BASE_EXPORT SchedulerThreadPool : public SchedulerWorkerThread::Delegate { On 2016/04/08 14:53:03, fdoray wrote: > On 2016/04/07 23:08:44, danakj wrote: > > nit: maybe consider putting the delegate in its own header file and not > nesting > > it (avoids this header needing to know about the whole SchedulerWorkerThread). > > I've been frustrated over time with public nested types in general, so just a > > feeling it might help you one day. feel free to leave it too. > > gab@: You asked me to nest Delegate inside SchedulerWorkerThread. Do you mind if > we move it back to its own .h file? I think it's fine as an inner class, @dana: was that your concern or more about it being merged direclty with the public API? And as we discussed you might even be able to put it all in the .cc file? https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:145: Do we need to verify that worker_threads_copy is still equal to |worker_threads| after joining? Otherwise we risk missing a thread created in between? https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:77: class SchedulerWorkerThreadDelegateImpl May even be able to only fwd-decl here? https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:94: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/ut... File base/task_scheduler/utils.h (right): https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/ut... base/task_scheduler/utils.h:23: bool BASE_EXPORT PostTaskHelper(std::unique_ptr<Task> task, This method could be private on SchedulerThreadPool with a public PostTaskHelperForTesting() mirror that just forwards to it for testing? Dana seemed to want it to be on STP as well?
https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:34: class BASE_EXPORT SchedulerThreadPool : public SchedulerWorkerThread::Delegate { On 2016/04/08 17:56:00, gab wrote: > On 2016/04/08 14:53:03, fdoray wrote: > > On 2016/04/07 23:08:44, danakj wrote: > > > nit: maybe consider putting the delegate in its own header file and not > > nesting > > > it (avoids this header needing to know about the whole > SchedulerWorkerThread). > > > I've been frustrated over time with public nested types in general, so just > a > > > feeling it might help you one day. feel free to leave it too. > > > > gab@: You asked me to nest Delegate inside SchedulerWorkerThread. Do you mind > if > > we move it back to its own .h file? > > I think it's fine as an inner class, @dana: was that your concern or more about > it being merged direclty with the public API? Not that, but that nested classes can't be forward declared, so they have weird implications for other files that want to use them. It's been annoying in the past in cc/ https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/ut... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/ut... base/task_scheduler/utils.cc:18: bool PostTaskHelper(std::unique_ptr<Task> task, On 2016/04/08 14:53:03, fdoray wrote: > On 2016/04/07 23:08:44, danakj wrote: > > Can this be a method on SchedulerThreadPool? > > > > That feels like it's advantageous in a few ways: > > - We don't leave scheduler_thread_pool.cc when we're posting tasks, easier to > > follow. > > - We don't have to pass around queue pointers and stuff, we just use the > > members. > > - That makes SchedulerThreadPool the only caller of BeginTransaction()->Push() > > right? Then maybe it can just call WakeUpOneThread on itself instead of going > > (SchedulerThreadPool or *TaskRunner) -> PriorityQueue::Transaction -> > > SchedulerThreadPool to wake up a thread. > > This method will also be used with single-threaded PriorityQueues. Why aren't single-threaded queues a SchedulerThreadPool with 1 thread? Maybe you can elaborate. > If I move it > to SchedulerThreadPool: > - |priority_queue| will still be an argument. > - single-threaded task runners declared in scheduler_worker_thread.cc will have > to know about SchedulerThreadPool. These single-thread ones wouldn't be waking up a thread I imagine, so maybe they could also separately be a place that calls Transaction::Push? I'm mostly trying to remove the callback for pushing and just do what work is in that callback at the callsite to Push(), since it's coming from the same class as the callback's destination. https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/ut... File base/task_scheduler/utils.h (right): https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/ut... base/task_scheduler/utils.h:23: bool BASE_EXPORT PostTaskHelper(std::unique_ptr<Task> task, On 2016/04/08 17:56:00, gab wrote: > This method could be private on SchedulerThreadPool with a public > PostTaskHelperForTesting() mirror that just forwards to it for testing? Dana > seemed to want it to be on STP as well? That, or protected with a TestSTP that makes it public with "using".
gab@/danakj@: Can you take another look? Let me know if you disagree with my arguments :) https://codereview.chromium.org/1708773002/diff/410001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1708773002/diff/410001/base/base.gypi#newcode649 base/base.gypi:649: 'task_scheduler/scheduler_worker_thread_delegate.h', On 2016/04/08 17:56:00, gab wrote: > Careful with doing merges in their own PS, the diff between 28 and 22 includes > something else and no PS indicate a merge. Looks like it's only affecting gyp in > this diff, but I'm always leery when I see something unrelated that the rest of > the diff I'm looking at is potentially including a merged CL... Ack https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:191: idle_worker_threads_stack_cv_->Broadcast(); On 2016/04/08 17:56:00, gab wrote: > On 2016/04/08 14:53:02, fdoray wrote: > > On 2016/04/07 20:32:47, gab wrote: > > > Would a manual-reset WaitableEvent be better here? It feels weird to make a > > > broadcast of something that might become false any second (I understand it > > > probably doesn't in the controlled way the test is set up but the reader of > > this > > > code doesn't know that). > > > > The WaitableEvent would be reset in WakeUpOneThread, when a thread is removed > > from the stack? I don't like this solution because it means that we'll have to > > think about resetting the event whenever we remove a thread from the stack > > (eventually, we'll also remove a thread from the stack when single-threaded > work > > is posted). Currently, WaitForAllWorkerThreadsIdleForTesting only returns when > > all threads are idle. This state can change immediately after, but it would > also > > be the case with a WaitableEvent. > > But the Broadcast's call can racily be lost, no? i.e. if the broadcast occurs > before the WaitForAllWorkerThreadsIdleForTesting call, the wait will block > forever? WaitForAllWorkerThreadsIdleForTesting will return immediately if all threads are idle, even if the broadcast is done before: while (idle_worker_threads_stack_.size() < worker_threads_.size()) <- this condition will be false idle_worker_threads_stack_cv_->Wait(); That's how ConditionVariable are supposed to be used https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati... https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:100: // constructor. On 2016/04/08 17:56:00, gab wrote: > On 2016/04/08 14:53:02, fdoray wrote: > > On 2016/04/07 20:32:48, gab wrote: > > > I don't think this last requirement "This is only modified by the > constructor" > > > is necessary. I definitely see a future world where the list of > > worker_threads_ > > > will be dynamic. Doesn't need to be now but no need to add this API explicit > > > requirement and restrict future evolutions of the impl. > > > > Done. Without this comment, I have to hold a lock to access |worker_threads_|. > > Good point, but I think that's the right thing to do (we'll have to long term > anyways). Ack. https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/410001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:297: TEST(TaskSchedulerPostTaskHelperTest, PostTaskInEmptySequence) { On 2016/04/08 17:56:00, gab wrote: > On 2016/04/08 14:53:03, fdoray wrote: > > On 2016/04/07 20:32:48, gab wrote: > > > Don't use the fixture name (TaskSchedulerPostTaskHelperTest) for tests that > > > aren't TEST_F > > > > Moved these tests to utils_unittest.cc. Not sure what your comment is supposed > > to mean. > > Oops, actually no I had somehow read "TaskSchedulerPostTaskHelperTest" as being > "TaskSchedulerThreadPoolTest"... "TaskSchedulerPostTaskHelperTest" is another > name and was correct, please do keep them here. I moved PostTaskHelper + tests to utils.cc/.h/_unitttest.cc because I plan to use it from single-threaded TaskRunners defined in scheduler_worker_thread.cc, not just because of your comment. https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:34: class BASE_EXPORT SchedulerThreadPool : public SchedulerWorkerThread::Delegate { On 2016/04/08 18:05:32, danakj wrote: > On 2016/04/08 17:56:00, gab wrote: > > On 2016/04/08 14:53:03, fdoray wrote: > > > On 2016/04/07 23:08:44, danakj wrote: > > > > nit: maybe consider putting the delegate in its own header file and not > > > nesting > > > > it (avoids this header needing to know about the whole > > SchedulerWorkerThread). > > > > I've been frustrated over time with public nested types in general, so > just > > a > > > > feeling it might help you one day. feel free to leave it too. > > > > > > gab@: You asked me to nest Delegate inside SchedulerWorkerThread. Do you > mind > > if > > > we move it back to its own .h file? > > > > I think it's fine as an inner class, @dana: was that your concern or more > about > > it being merged direclty with the public API? > > Not that, but that nested classes can't be forward declared, so they have weird > implications for other files that want to use them. It's been annoying in the > past in cc/ I'll move SchedulerWorkerThread::Delegate to its own header file in a separate CL. https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/ut... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/ut... base/task_scheduler/utils.cc:18: bool PostTaskHelper(std::unique_ptr<Task> task, On 2016/04/08 18:05:32, danakj wrote: > On 2016/04/08 14:53:03, fdoray wrote: > > On 2016/04/07 23:08:44, danakj wrote: > > > Can this be a method on SchedulerThreadPool? > > > > > > That feels like it's advantageous in a few ways: > > > - We don't leave scheduler_thread_pool.cc when we're posting tasks, easier > to > > > follow. > > > - We don't have to pass around queue pointers and stuff, we just use the > > > members. > > > - That makes SchedulerThreadPool the only caller of > BeginTransaction()->Push() > > > right? Then maybe it can just call WakeUpOneThread on itself instead of > going > > > (SchedulerThreadPool or *TaskRunner) -> PriorityQueue::Transaction -> > > > SchedulerThreadPool to wake up a thread. > > > > This method will also be used with single-threaded PriorityQueues. > > Why aren't single-threaded queues a SchedulerThreadPool with 1 thread? Maybe you > can elaborate. > > > If I move it > > to SchedulerThreadPool: > > - |priority_queue| will still be an argument. > > - single-threaded task runners declared in scheduler_worker_thread.cc will > have > > to know about SchedulerThreadPool. > > These single-thread ones wouldn't be waking up a thread I imagine, so maybe they > could also separately be a place that calls Transaction::Push? > > I'm mostly trying to remove the callback for pushing and just do what work is in > that callback at the callsite to Push(), since it's coming from the same class > as the callback's destination. Each thread in a pool will get work from both the pool's shared PQ and from its single-threaded PQ. PostTaskHelper can be used to post a task to either kind of PQ. Currently, you see PostTaskHelper being used in scheduler_thread_pool.cc, but it will also be used in scheduler_worker_thread.cc (by SchedulerSingleThreadTaskRunner) and delayed_task_manager.cc (to insert a task in its sequence/PQ when its delay expires). Because PQ has a callback to wake up a thread when a Sequence is inserted into it, delayed_task_manager.cc will be able to use shared/single-thread PQs without knowing the difference between the two. Without the callback, delayed_task_manager.cc will have to know that it has to call SchedulerThreadPool::WakeUpOneThread when it inserts a Sequence in a shared PQ and SchedulerWorkerThread::WakeUp when it inserts a Sequence in a single-threaded PQ. https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:145: On 2016/04/08 17:56:00, gab wrote: > Do we need to verify that worker_threads_copy is still equal to |worker_threads| > after joining? Otherwise we risk missing a thread created in between? I would like to put back the comment that says that |worker_threads_| is only modified during initialization and remove the copy here. I would like to implement ramp up / down without changing |worker_threads_|. SchedulerWorkerThreads would be created with their SchedulerThreadPool and would never be deleted. The thread that they manage internally could however exit and be re-created on WakeUp(). https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:77: class SchedulerWorkerThreadDelegateImpl On 2016/04/08 17:56:00, gab wrote: > May even be able to only fwd-decl here? Done. https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:94: }; On 2016/04/08 17:56:00, gab wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/ut... File base/task_scheduler/utils.h (right): https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/ut... base/task_scheduler/utils.h:23: bool BASE_EXPORT PostTaskHelper(std::unique_ptr<Task> task, On 2016/04/08 18:05:32, danakj wrote: > On 2016/04/08 17:56:00, gab wrote: > > This method could be private on SchedulerThreadPool with a public > > PostTaskHelperForTesting() mirror that just forwards to it for testing? Dana > > seemed to want it to be on STP as well? > > That, or protected with a TestSTP that makes it public with "using". I don't think that this helper should be in STP. I want to be able to use it to insert work in a single-threaded PQ in scheduler_worker_thread.cc, which shouldn't depend on scheduler_thread_pool.h.
lgtm w/ nits (didn't review utils*, assuming it's just a cut/paste from the previously reviewed code/tests) https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:34: class BASE_EXPORT SchedulerThreadPool : public SchedulerWorkerThread::Delegate { On 2016/04/08 18:05:32, danakj wrote: > On 2016/04/08 17:56:00, gab wrote: > > On 2016/04/08 14:53:03, fdoray wrote: > > > On 2016/04/07 23:08:44, danakj wrote: > > > > nit: maybe consider putting the delegate in its own header file and not > > > nesting > > > > it (avoids this header needing to know about the whole > > SchedulerWorkerThread). > > > > I've been frustrated over time with public nested types in general, so > just > > a > > > > feeling it might help you one day. feel free to leave it too. > > > > > > gab@: You asked me to nest Delegate inside SchedulerWorkerThread. Do you > mind > > if > > > we move it back to its own .h file? > > > > I think it's fine as an inner class, @dana: was that your concern or more > about > > it being merged direclty with the public API? > > Not that, but that nested classes can't be forward declared, so they have weird > implications for other files that want to use them. It's been annoying in the > past in cc/ Hmmm but why would another file want to use the Delegate? It's an implementation detail of this class (i.e. I think we can put it all in the .cc file except for a mere fwd-decl in this header to declare it as an inner class). https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/530001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:145: On 2016/04/08 19:00:05, fdoray wrote: > On 2016/04/08 17:56:00, gab wrote: > > Do we need to verify that worker_threads_copy is still equal to > |worker_threads| > > after joining? Otherwise we risk missing a thread created in between? > > I would like to put back the comment that says that |worker_threads_| is only > modified during initialization and remove the copy here. > > I would like to implement ramp up / down without changing |worker_threads_|. > SchedulerWorkerThreads would be created with their SchedulerThreadPool and would > never be deleted. The thread that they manage internally could however exit and > be re-created on WakeUp(). Okay, feel free to put it back for now and we'll see when we get to that CL. https://codereview.chromium.org/1708773002/diff/550001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/550001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:101: SchedulerLock worker_threads_lock_; Change the name back to |idle_worker_threads_lock_| if it's specific to them again?
danakj@: Can you take another look? Thanks. https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:34: class BASE_EXPORT SchedulerThreadPool : public SchedulerWorkerThread::Delegate { On 2016/04/08 21:10:50, gab wrote: > On 2016/04/08 18:05:32, danakj wrote: > > On 2016/04/08 17:56:00, gab wrote: > > > On 2016/04/08 14:53:03, fdoray wrote: > > > > On 2016/04/07 23:08:44, danakj wrote: > > > > > nit: maybe consider putting the delegate in its own header file and not > > > > nesting > > > > > it (avoids this header needing to know about the whole > > > SchedulerWorkerThread). > > > > > I've been frustrated over time with public nested types in general, so > > just > > > a > > > > > feeling it might help you one day. feel free to leave it too. > > > > > > > > gab@: You asked me to nest Delegate inside SchedulerWorkerThread. Do you > > mind > > > if > > > > we move it back to its own .h file? > > > > > > I think it's fine as an inner class, @dana: was that your concern or more > > about > > > it being merged direclty with the public API? > > > > Not that, but that nested classes can't be forward declared, so they have > weird > > implications for other files that want to use them. It's been annoying in the > > past in cc/ > > Hmmm but why would another file want to use the Delegate? It's an implementation > detail of this class (i.e. I think we can put it all in the .cc file except for > a mere fwd-decl in this header to declare it as an inner class). I will move the definition of *SchedulerWorkerThread::Delegate* (base class) to its own file. The definition of SchedulerWorkerThreadDelegateImpl will stay in scheduler_thread_pool.cc. https://codereview.chromium.org/1708773002/diff/550001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/550001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:101: SchedulerLock worker_threads_lock_; On 2016/04/08 21:10:51, gab wrote: > Change the name back to |idle_worker_threads_lock_| if it's specific to them > again? Done.
danakj@: Can you take another look? Thanks. https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/ut... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1708773002/diff/470001/base/task_scheduler/ut... base/task_scheduler/utils.cc:18: bool PostTaskHelper(std::unique_ptr<Task> task, On 2016/04/08 19:00:05, fdoray wrote: > On 2016/04/08 18:05:32, danakj wrote: > > On 2016/04/08 14:53:03, fdoray wrote: > > > On 2016/04/07 23:08:44, danakj wrote: > > > > Can this be a method on SchedulerThreadPool? > > > > > > > > That feels like it's advantageous in a few ways: > > > > - We don't leave scheduler_thread_pool.cc when we're posting tasks, easier > > to > > > > follow. > > > > - We don't have to pass around queue pointers and stuff, we just use the > > > > members. > > > > - That makes SchedulerThreadPool the only caller of > > BeginTransaction()->Push() > > > > right? Then maybe it can just call WakeUpOneThread on itself instead of > > going > > > > (SchedulerThreadPool or *TaskRunner) -> PriorityQueue::Transaction -> > > > > SchedulerThreadPool to wake up a thread. > > > > > > This method will also be used with single-threaded PriorityQueues. > > > > Why aren't single-threaded queues a SchedulerThreadPool with 1 thread? Maybe > you > > can elaborate. > > > > > If I move it > > > to SchedulerThreadPool: > > > - |priority_queue| will still be an argument. > > > - single-threaded task runners declared in scheduler_worker_thread.cc will > > have > > > to know about SchedulerThreadPool. > > > > These single-thread ones wouldn't be waking up a thread I imagine, so maybe > they > > could also separately be a place that calls Transaction::Push? > > > > I'm mostly trying to remove the callback for pushing and just do what work is > in > > that callback at the callsite to Push(), since it's coming from the same class > > as the callback's destination. > > Each thread in a pool will get work from both the pool's shared PQ and from its > single-threaded PQ. PostTaskHelper can be used to post a task to either kind of > PQ. > > Currently, you see PostTaskHelper being used in scheduler_thread_pool.cc, but it > will also be used in scheduler_worker_thread.cc (by > SchedulerSingleThreadTaskRunner) and delayed_task_manager.cc (to insert a task > in its sequence/PQ when its delay expires). > > Because PQ has a callback to wake up a thread when a Sequence is inserted into > it, delayed_task_manager.cc will be able to use shared/single-thread PQs without > knowing the difference between the two. Without the callback, > delayed_task_manager.cc will have to know that it has to call > SchedulerThreadPool::WakeUpOneThread when it inserts a Sequence in a shared PQ > and SchedulerWorkerThread::WakeUp when it inserts a Sequence in a > single-threaded PQ. Usage of this helper in DelayedTaskManager: https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de...
danakj@: Can you take another look? Post non-delayed task: TaskRunner::PostTask PostTaskHelper SchedulerTaskExecutor::PostTaskNow (STP or SWT) PostTaskNowHelper Post delayed task: TaskRunner::PostTask PostTaskHelper DelayedTaskManager::AddDelayedTask When delayed task becomes ripe for execution: DelayedTaskManager::PostReadyTasks SchedulerTaskExecutor::PostTaskNow (STP or SWT) PostTaskNowHelper Why do we need PostTaskHelper? We prefer to have one call site for TaskTracker::WillPostTask than to duplicate this call in our three types of TaskRunners. This helper will also be the only call site for DelayedTaskManager::AddDelayedTask. Why do we need PostTaskNowHelper? This helper will be used by SchedulerThreadPool::PostTaskNow and SchedulerWorkerThread::PostTaskNow. We prefer not to duplicate this code and the associated comment in these two classes.
On 2016/04/13 14:37:18, fdoray wrote: > danakj@: Can you take another look? > > Post non-delayed task: > TaskRunner::PostTask > PostTaskHelper > SchedulerTaskExecutor::PostTaskNow (STP or SWT) > PostTaskNowHelper > > Post delayed task: > TaskRunner::PostTask > PostTaskHelper > DelayedTaskManager::AddDelayedTask > > When delayed task becomes ripe for execution: > DelayedTaskManager::PostReadyTasks > SchedulerTaskExecutor::PostTaskNow (STP or SWT) > PostTaskNowHelper > > Why do we need PostTaskHelper? We prefer to have one call site for > TaskTracker::WillPostTask than to duplicate this call in our three types of > TaskRunners. This helper will also be the only call site for > DelayedTaskManager::AddDelayedTask. > > Why do we need PostTaskNowHelper? This helper will be used by > SchedulerThreadPool::PostTaskNow and SchedulerWorkerThread::PostTaskNow. We > prefer not to duplicate this code and the associated comment in these two > classes. Enough things have changed that I'm going to re-review from the top. Marking Not LGTM for now.
On 2016/04/13 17:56:39, robliao wrote: > On 2016/04/13 14:37:18, fdoray wrote: > > danakj@: Can you take another look? > > > > Post non-delayed task: > > TaskRunner::PostTask > > PostTaskHelper > > SchedulerTaskExecutor::PostTaskNow (STP or SWT) > > PostTaskNowHelper > > > > Post delayed task: > > TaskRunner::PostTask > > PostTaskHelper > > DelayedTaskManager::AddDelayedTask > > > > When delayed task becomes ripe for execution: > > DelayedTaskManager::PostReadyTasks > > SchedulerTaskExecutor::PostTaskNow (STP or SWT) > > PostTaskNowHelper > > > > Why do we need PostTaskHelper? We prefer to have one call site for > > TaskTracker::WillPostTask than to duplicate this call in our three types of > > TaskRunners. This helper will also be the only call site for > > DelayedTaskManager::AddDelayedTask. > > > > Why do we need PostTaskNowHelper? This helper will be used by > > SchedulerThreadPool::PostTaskNow and SchedulerWorkerThread::PostTaskNow. We > > prefer not to duplicate this code and the associated comment in these two > > classes. > > Enough things have changed that I'm going to re-review from the top. Marking Not > LGTM for now. Re-lgtm. Some notes here and there.
On 2016/04/13 17:56:39, robliao wrote: > On 2016/04/13 14:37:18, fdoray wrote: > > danakj@: Can you take another look? > > > > Post non-delayed task: > > TaskRunner::PostTask > > PostTaskHelper > > SchedulerTaskExecutor::PostTaskNow (STP or SWT) > > PostTaskNowHelper > > > > Post delayed task: > > TaskRunner::PostTask > > PostTaskHelper > > DelayedTaskManager::AddDelayedTask > > > > When delayed task becomes ripe for execution: > > DelayedTaskManager::PostReadyTasks > > SchedulerTaskExecutor::PostTaskNow (STP or SWT) > > PostTaskNowHelper > > > > Why do we need PostTaskHelper? We prefer to have one call site for > > TaskTracker::WillPostTask than to duplicate this call in our three types of > > TaskRunners. This helper will also be the only call site for > > DelayedTaskManager::AddDelayedTask. > > > > Why do we need PostTaskNowHelper? This helper will be used by > > SchedulerThreadPool::PostTaskNow and SchedulerWorkerThread::PostTaskNow. We > > prefer not to duplicate this code and the associated comment in these two > > classes. > > Enough things have changed that I'm going to re-review from the top. Marking Not > LGTM for now. Re-lgtm. Some notes here and there.
Let's try this again. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... File base/task_scheduler/scheduler_task_executor.h (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_task_executor.h:23: // delayed run time of |task| is ignored. The scheduler's TaskTracker must Ignoring the delayed run time is an implementation detail of components that implement SchedulerTaskExecutor. I would remove the sentence covering that portion. Given that the Now could be ignored later, maybe PostTaskWithSequence? https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:237: { It might be cleaner to take the below and encapsulate it in PopOneIdleThread. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:56: ~SchedulerThreadPool() override; Nit: This should go above the static function above. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:99: // All worker threads owned by this thread pool. Is only modified during Nit: "It is only modified" or "Only modified" https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:60: // TaskScheduler which would first determine in which PriorityQueue the Nit: Remove "in" https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:185: for (size_t j = 0; j < kNumThreadsPostingTasks; ++j) { Nit: size_t i, here and below. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:187: thread_pool_.get(), ExecutionMode::PARALLEL, false, false))); Optional: To make "false, false" more readable without creating an enum, assign the vals to local vars and then pass those local vars in.
danakj@: Can you take another look? Post non-delayed task: TaskRunner::PostTask PostTaskHelper SchedulerTaskExecutor::PostTaskWithSequence (STP or SWT) PostTaskWithSequenceHelper Post delayed task: TaskRunner::PostTask PostTaskHelper DelayedTaskManager::AddDelayedTask When delayed task becomes ripe for execution: DelayedTaskManager::PostReadyTasks SchedulerTaskExecutor::PostTaskWithSequence (STP or SWT) PostTaskWithSequenceHelper Why do we need PostTaskHelper? We prefer to have one call site for TaskTracker::WillPostTask than to duplicate this call in our three types of TaskRunners. This helper will also be the only call site for DelayedTaskManager::AddDelayedTask. Why do we need PostTaskWithSequenceHelper? This helper will be used by SchedulerThreadPool::PostTaskWithSequence and SchedulerWorkerThread::PostTaskWithSequence. We prefer not to duplicate the code of this helper in these two classes. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... File base/task_scheduler/scheduler_task_executor.h (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_task_executor.h:23: // delayed run time of |task| is ignored. The scheduler's TaskTracker must On 2016/04/13 20:50:54, robliao wrote: > Ignoring the delayed run time is an implementation detail of components that > implement SchedulerTaskExecutor. I would remove the sentence covering that > portion. > > Given that the Now could be ignored later, maybe PostTaskWithSequence? Done. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:237: { On 2016/04/13 20:50:54, robliao wrote: > It might be cleaner to take the below and encapsulate it in PopOneIdleThread. Done. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:56: ~SchedulerThreadPool() override; On 2016/04/13 20:50:55, robliao wrote: > Nit: This should go above the static function above. Done. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:99: // All worker threads owned by this thread pool. Is only modified during On 2016/04/13 20:50:55, robliao wrote: > Nit: "It is only modified" or "Only modified" Done. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:60: // TaskScheduler which would first determine in which PriorityQueue the On 2016/04/13 20:50:55, robliao wrote: > Nit: Remove "in" Done. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:185: for (size_t j = 0; j < kNumThreadsPostingTasks; ++j) { On 2016/04/13 20:50:55, robliao wrote: > Nit: size_t i, here and below. Done. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_unittest.cc:187: thread_pool_.get(), ExecutionMode::PARALLEL, false, false))); On 2016/04/13 20:50:55, robliao wrote: > Optional: To make "false, false" more readable without creating an enum, assign > the vals to local vars and then pass those local vars in. Done.
Some comments, but still lgtm overall, thanks! https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:237: { On 2016/04/13 23:13:12, fdoray wrote: > On 2016/04/13 20:50:54, robliao wrote: > > It might be cleaner to take the below and encapsulate it in PopOneIdleThread. > > Done. I disagree, now this is one more hop for the reader, effectively this scope was an inlined anonymous method with an unambiguous purpose (i.e. didn't require an explicit name to be understandable). I prefer this Patch Set's way to PS32's. Not feeling very strongly, but less methods makes for an easier read IMO and this extra one doesn't feel necessary. https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:23: // don't belong to a SchedulerThreadPool. s/. Not set for threads that.../, if any./ https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:46: return tls_current_thread_pool.Get().Get() == executor_; I think this requires a static_cast<SchedulerTaskExecutor> on the TLS pointer otherwise the pointer may be an offset to the subclass' virtual table and may not match even when it's the same underlying STP (I'm not an expert here but I think this is the case). Or we could make the TLS pointer a SchedulerTaskExecutor (and then I guess you need to cast on the comparison on line 137..). https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:53: SchedulerTaskExecutor* const executor_; Ah, here it is, I knew we had already made another assumption about lifetime. This code (and the one in previous PatchSets on this CL) assume STP is alive forever. Should document that where necessary. https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:267: DCHECK_GE(TimeTicks::Now(), task->delayed_run_time); What is this verifying? From tracking_info.h, |delayed_run_time| is the time when the task is scheduled to run, why does that have to be before Now()? (also I find inequality checks read better when the "constant", i.e. Now(), is on the RHS of the comparison) Also, as discussed offline, right now task.cc initializes that to null so this is always true, but you said this would change later but this DCHECK would stick so that's okay. https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:97: void PostTaskWithSequence(std::unique_ptr<Task> task, Why is this private? I guess the caller is using the SchedulerTaskExecutor interface and thus making a public call to it, but this being private may lead the reader to think it's never called from outside this component which it definitely is (this is not the same IMO as a private call which is bound and given to a public caller, now this is trivially made public through a cast by anyone). https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/ut... File base/task_scheduler/utils_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/ut... base/task_scheduler/utils_unittest.cc:22: Can we also test PostTaskHelper?
https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:237: { On 2016/04/14 16:11:34, gab wrote: > On 2016/04/13 23:13:12, fdoray wrote: > > On 2016/04/13 20:50:54, robliao wrote: > > > It might be cleaner to take the below and encapsulate it in > PopOneIdleThread. > > > > Done. > > I disagree, now this is one more hop for the reader, effectively this scope was > an inlined anonymous method with an unambiguous purpose (i.e. didn't require an > explicit name to be understandable). I prefer this Patch Set's way to PS32's. > > Not feeling very strongly, but less methods makes for an easier read IMO and > this extra one doesn't feel necessary. The reason why I like the separation is that it keeps methods either fully synchronized or not, making it easy to check concurrency.
https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:46: return tls_current_thread_pool.Get().Get() == executor_; On 2016/04/14 16:11:35, gab wrote: > I think this requires a static_cast<SchedulerTaskExecutor> on the TLS pointer > otherwise the pointer may be an offset to the subclass' virtual table and may > not match even when it's the same underlying STP (I'm not an expert here but I > think this is the case). > > Or we could make the TLS pointer a SchedulerTaskExecutor (and then I guess you > need to cast on the comparison on line 137..). C++ operator== requires that the types of both sides be equivalent before performing the compare (operator== const X& lhs, const X& rhs) This means both the left and right sides must be convertable to the same type to make the comparison, so the cast is already there, otherwise we can't see it. Consider the following: class Root {}; class Root2 {}; class Multiple : public Root, public Root2 {}; Multiple* m = new Multiple; Root* r = m; Root2* r2 = m; r == r2 // Compile Error: Root* and Root2* are not convertable between each other. m == r // true - Multiple* is castable to Root* m == r2 // true - Multiple* is castable to Root2*. static_cast<void*>(m) == static_cast<void*>(r2) // false - In my compiler's implementation, m and r2 point to different portions of the object.
https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:46: return tls_current_thread_pool.Get().Get() == executor_; On 2016/04/14 17:06:28, robliao wrote: > On 2016/04/14 16:11:35, gab wrote: > > I think this requires a static_cast<SchedulerTaskExecutor> on the TLS pointer > > otherwise the pointer may be an offset to the subclass' virtual table and may > > not match even when it's the same underlying STP (I'm not an expert here but I > > think this is the case). > > > > Or we could make the TLS pointer a SchedulerTaskExecutor (and then I guess you > > need to cast on the comparison on line 137..). > > C++ operator== requires that the types of both sides be equivalent before > performing the compare (operator== const X& lhs, const X& rhs) > > This means both the left and right sides must be convertable to the same type to > make the comparison, so the cast is already there, otherwise we can't see it. > > Consider the following: > > class Root {}; > class Root2 {}; > class Multiple : public Root, public Root2 {}; > > Multiple* m = new Multiple; > Root* r = m; > Root2* r2 = m; > > r == r2 // Compile Error: Root* and Root2* are not convertable between each > other. > > m == r // true - Multiple* is castable to Root* > > m == r2 // true - Multiple* is castable to Root2*. > > static_cast<void*>(m) == static_cast<void*>(r2) // false - In my compiler's > implementation, m and r2 point to different portions of the object. > > Ah ok, then all is good :-). Shall we make the TLS state only a SchedulerTaskExecutor pointer though then since we don't need it to be a full STP?
danakj@: Can you take another look? Post non-delayed task: TaskRunner::PostTask PostTaskHelper SchedulerTaskExecutor::PostTaskWithSequence (STP or SWT) PostTaskWithSequenceHelper Post delayed task: TaskRunner::PostTask PostTaskHelper DelayedTaskManager::AddDelayedTask When delayed task becomes ripe for execution: DelayedTaskManager::PostReadyTasks SchedulerTaskExecutor::PostTaskWithSequence (STP or SWT) PostTaskWithSequenceHelper Why do we need PostTaskHelper? We prefer to have one call site for TaskTracker::WillPostTask than to duplicate this call in our three types of TaskRunners. This helper will also be the only call site for DelayedTaskManager::AddDelayedTask. Why do we need PostTaskWithSequenceHelper? This helper will be used by SchedulerThreadPool::PostTaskWithSequence and SchedulerWorkerThread::PostTaskWithSequence. We prefer not to duplicate the code of this helper in these two classes. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:237: { On 2016/04/14 16:43:38, robliao wrote: > On 2016/04/14 16:11:34, gab wrote: > > On 2016/04/13 23:13:12, fdoray wrote: > > > On 2016/04/13 20:50:54, robliao wrote: > > > > It might be cleaner to take the below and encapsulate it in > > PopOneIdleThread. > > > > > > Done. > > > > I disagree, now this is one more hop for the reader, effectively this scope > was > > an inlined anonymous method with an unambiguous purpose (i.e. didn't require > an > > explicit name to be understandable). I prefer this Patch Set's way to PS32's. > > > > Not feeling very strongly, but less methods makes for an easier read IMO and > > this extra one doesn't feel necessary. > > The reason why I like the separation is that it keeps methods either fully > synchronized or not, making it easy to check concurrency. Keeping the code as-is because I plan to create a custom stack that supports atomic Peek+Pop anyway (the custom stack will support removal of arbitrary elements, which will be useful to remove threads from the stack when they are woken up for single-threaded work). https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:23: // don't belong to a SchedulerThreadPool. On 2016/04/14 16:11:34, gab wrote: > s/. Not set for threads that.../, if any./ Done. https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:46: return tls_current_thread_pool.Get().Get() == executor_; On 2016/04/14 18:28:21, gab wrote: > On 2016/04/14 17:06:28, robliao wrote: > > On 2016/04/14 16:11:35, gab wrote: > > > I think this requires a static_cast<SchedulerTaskExecutor> on the TLS > pointer > > > otherwise the pointer may be an offset to the subclass' virtual table and > may > > > not match even when it's the same underlying STP (I'm not an expert here but > I > > > think this is the case). > > > > > > Or we could make the TLS pointer a SchedulerTaskExecutor (and then I guess > you > > > need to cast on the comparison on line 137..). > > > > C++ operator== requires that the types of both sides be equivalent before > > performing the compare (operator== const X& lhs, const X& rhs) > > > > This means both the left and right sides must be convertable to the same type > to > > make the comparison, so the cast is already there, otherwise we can't see it. > > > > Consider the following: > > > > class Root {}; > > class Root2 {}; > > class Multiple : public Root, public Root2 {}; > > > > Multiple* m = new Multiple; > > Root* r = m; > > Root2* r2 = m; > > > > r == r2 // Compile Error: Root* and Root2* are not convertable between each > > other. > > > > m == r // true - Multiple* is castable to Root* > > > > m == r2 // true - Multiple* is castable to Root2*. > > > > static_cast<void*>(m) == static_cast<void*>(r2) // false - In my compiler's > > implementation, m and r2 point to different portions of the object. > > > > > > Ah ok, then all is good :-). > > Shall we make the TLS state only a SchedulerTaskExecutor pointer though then > since we don't need it to be a full STP? Done. Made the TLS a SchedulerTaskExecutor*. https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:53: SchedulerTaskExecutor* const executor_; On 2016/04/14 16:11:35, gab wrote: > Ah, here it is, I knew we had already made another assumption about lifetime. > This code (and the one in previous PatchSets on this CL) assume STP is alive > forever. > > Should document that where necessary. Done. Added comment. https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:267: DCHECK_GE(TimeTicks::Now(), task->delayed_run_time); On 2016/04/14 16:11:34, gab wrote: > What is this verifying? From tracking_info.h, |delayed_run_time| is the time > when the task is scheduled to run, why does that have to be before Now()? > > (also I find inequality checks read better when the "constant", i.e. Now(), is > on the RHS of the comparison) > > Also, as discussed offline, right now task.cc initializes that to null so this > is always true, but you said this would change later but this DCHECK would stick > so that's okay. Added comment to explain the purpose of this check. https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:97: void PostTaskWithSequence(std::unique_ptr<Task> task, On 2016/04/14 16:11:35, gab wrote: > Why is this private? I guess the caller is using the SchedulerTaskExecutor > interface and thus making a public call to it, but this being private may lead > the reader to think it's never called from outside this component which it > definitely is (this is not the same IMO as a private call which is bound and > given to a public caller, now this is trivially made public through a cast by > anyone). I made the method public because I agree with you that other components could call it through a SchedulerTaskExecutor*. If the base class had been ComponentADelegate, I would have preferred to keep the method private to document that it shouldn't be called outside of ComponentA (something is clearly wrong if ComponentB has to cast a SchedulerThreadPool* to ComponentADelegate* to call one of its method). Relevant discussion: https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/CwzjcWd9cYM... https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/ut... File base/task_scheduler/utils_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/ut... base/task_scheduler/utils_unittest.cc:22: On 2016/04/14 16:11:35, gab wrote: > Can we also test PostTaskHelper? Done.
Minor comments https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1708773002/diff/610001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:97: void PostTaskWithSequence(std::unique_ptr<Task> task, On 2016/04/14 18:41:12, fdoray wrote: > On 2016/04/14 16:11:35, gab wrote: > > Why is this private? I guess the caller is using the SchedulerTaskExecutor > > interface and thus making a public call to it, but this being private may lead > > the reader to think it's never called from outside this component which it > > definitely is (this is not the same IMO as a private call which is bound and > > given to a public caller, now this is trivially made public through a cast by > > anyone). > > I made the method public because I agree with you that other components could > call it through a SchedulerTaskExecutor*. If the base class had been > ComponentADelegate, I would have preferred to keep the method private to > document that it shouldn't be called outside of ComponentA (something is clearly > wrong if ComponentB has to cast a SchedulerThreadPool* to ComponentADelegate* to > call one of its method). > > Relevant discussion: > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/CwzjcWd9cYM... Right I agree for Delegates because they are single-purposed (i.e. typically handed off to a few known classes, in that sense it's akin to Bind'ing a private method a bit). In this case however many things will hold on to SchedulerTaskExecutor and post to them so it feels right to make the method public to make it clear the call can come from anywhere... (I think this is also why you agree but just reply'ing for the purpose of discussion :-)) https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:56: // to spread this assumption throughout the scheduler. A WeakPtr isn't ever possible here as PostTask is clearly called from multiple threads and therefore it's impossible to have all lookups be on the same thread (as it potentially was for the instance in DelayedTaskManager). Maybe something like this on the constructor instead: // Constructs a SchedulerParallelTaskRunner which can be used to post tasks so long as |executor| is alive. // TODO(robliao): Find a concrete way to manage |executor|'s memory. SchedulerParallelTaskRunner(... https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:161: // Verify that we are passed |task|'s delayed run time (i.e. |task| is ready s/passed/past/ rm "we", e.g.: // Confirm that |task| is ready to run (its delayed run time is either null or in the past). https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ta... File base/task_scheduler/task_traits.cc (right): https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ta... base/task_scheduler/task_traits.cc:38: return with_file_io_ == other.with_file_io_ && priority_ == other.priority_ && Could we do something like static_assert(sizeof(TaskTraits) == whatever-it-is, "TaskTraits size changed, verify that operator=='s implementation is still valid.") ? This probably would not work across compiler toolchains and all with a fixed value but I'd really like to find a way to make sure this is updated when new fields are added to the struct... @Rob: ideas here? Or we make this a test only interface by moving the operator== outside the class into the anonymous namespace of the test that needs it to be defined. https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ut... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ut... base/task_scheduler/utils.cc:54: DCHECK_LE(task->delayed_run_time, TimeTicks::Now()); Since the same DCHECK is already in PostTaskWithSequence can we keep only one of them and get the same coverage? I'm guessing this one has even more coverage per being in the helper method? https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ut... File base/task_scheduler/utils_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ut... base/task_scheduler/utils_unittest.cc:68: } Explicitly VerifyAndClear strict mock at the end? It wasn't initially clear to me how the lack of a call was being verified.
LGTM w/ a few comments. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:158: const EnqueueSequenceCallback enqueue_sequence_callback) const&? https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:164: void SchedulerThreadPool::SchedulerWorkerThreadDelegateImpl::OnMainEntry() { Just in case, if these are in a DelegateImpl and not just on SchedulerThreadPool because of previous talking about moving the Delegate to not be a nested class (where I meant the delegate definition itself), you could put them back, not sure. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... base/task_scheduler/utils.cc:52: const bool sequence_was_empty = sequence->PushTask(std::move(task)); Is the assumption that Sequence is already in priority_queue if it wasn't empty? Should we DCHECK that the pointer is the right one? https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... File base/task_scheduler/utils.h (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... base/task_scheduler/utils.h:28: // Helper for posting a task to the provided |sequence| and |executor| "Helper for posting" -> "Attempts to post" https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... base/task_scheduler/utils.h:30: bool BASE_EXPORT PostTaskHelper(const tracked_objects::Location& posted_from, Can you name this more descriptively based on what it does? "Helper" is just a placeholder word. {Try?}PostTaskToExecutor? Or something else.. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... base/task_scheduler/utils.h:38: // Helper for posting |task| to the provided |sequence| and |priority_queue|. ditto https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... base/task_scheduler/utils.h:40: bool BASE_EXPORT PostTaskNowHelper(std::unique_ptr<Task> task, Ditto, "Helper" is not helpful in telling me what will happen when I call this function. AddTaskToSequenceInPriorityQueue?
https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ta... File base/task_scheduler/task_traits.cc (right): https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ta... base/task_scheduler/task_traits.cc:38: return with_file_io_ == other.with_file_io_ && priority_ == other.priority_ && On 2016/04/14 19:34:16, gab wrote: > Could we do something like static_assert(sizeof(TaskTraits) == whatever-it-is, > "TaskTraits size changed, verify that operator=='s implementation is still > valid.") > ? > > This probably would not work across compiler toolchains and all with a fixed > value but I'd really like to find a way to make sure this is updated when new > fields are added to the struct... > > @Rob: ideas here? > > Or we make this a test only interface by moving the operator== outside the class > into the anonymous namespace of the test that needs it to be defined. Adding discussion notes from chat: This appears to work: static_assert(offsetof(TaskTraits, shutdown_behavior_) + sizeof(TaskTraits::shutdown_behavior_) == sizeof(TaskTraits), "Size Changed. Update operator=="); You designate the last expected value and check that your size and size from the expected value are the same. But if we move operator== outside, we'll need to friend function it.
Thanks for the reviews! https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:158: const EnqueueSequenceCallback enqueue_sequence_callback) On 2016/04/14 19:48:58, danakj wrote: > const&? Done. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:164: void SchedulerThreadPool::SchedulerWorkerThreadDelegateImpl::OnMainEntry() { On 2016/04/14 19:48:58, danakj wrote: > Just in case, if these are in a DelegateImpl and not just on SchedulerThreadPool > because of previous talking about moving the Delegate to not be a nested class > (where I meant the delegate definition itself), you could put them back, not > sure. These methods are in DelegateImpl instead of SchedulerThreadPool because we didn't want 2 methods called EnqueueSequence on SchedulerThreadPool. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... base/task_scheduler/utils.cc:52: const bool sequence_was_empty = sequence->PushTask(std::move(task)); On 2016/04/14 19:48:58, danakj wrote: > Is the assumption that Sequence is already in priority_queue if it wasn't empty? > Should we DCHECK that the pointer is the right one? If |sequence| wasn't empty, it is already in a PriorityQueue (which isn't necessarily |priority_queue| [1]) OR a worker thread is running a task from it and will insert it in a PriorityQueue when it's done. I clarified the comment below. [1] If |sequence| contains BACKGROUND tasks only, it might be in the PQ of a background thread pool. If |task| is USER_VISIBLE, |priority_queue| is the PQ of a high priority thread pool. |sequence| won't be inserted in |priority_queue| by this function; it will remain in the PQ of the background thread pool until one of its tasks run. Then, it will be pushed to the PQ of the high priority thread pool (on re-insert, we use the PQ associated with the highest priority in the Sequence). https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... File base/task_scheduler/utils.h (right): https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... base/task_scheduler/utils.h:28: // Helper for posting a task to the provided |sequence| and |executor| On 2016/04/14 19:48:58, danakj wrote: > "Helper for posting" -> "Attempts to post" Done. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... base/task_scheduler/utils.h:30: bool BASE_EXPORT PostTaskHelper(const tracked_objects::Location& posted_from, On 2016/04/14 19:48:58, danakj wrote: > Can you name this more descriptively based on what it does? "Helper" is just a > placeholder word. > > {Try?}PostTaskToExecutor? Or something else.. Done. Renamed it PostTaskToExecutor. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... base/task_scheduler/utils.h:38: // Helper for posting |task| to the provided |sequence| and |priority_queue|. On 2016/04/14 19:48:58, danakj wrote: > ditto Done. https://codereview.chromium.org/1708773002/diff/590001/base/task_scheduler/ut... base/task_scheduler/utils.h:40: bool BASE_EXPORT PostTaskNowHelper(std::unique_ptr<Task> task, On 2016/04/14 19:48:58, danakj wrote: > Ditto, "Helper" is not helpful in telling me what will happen when I call this > function. AddTaskToSequenceInPriorityQueue? Done. https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:56: // to spread this assumption throughout the scheduler. On 2016/04/14 19:34:16, gab wrote: > A WeakPtr isn't ever possible here as PostTask is clearly called from multiple > threads and therefore it's impossible to have all lookups be on the same thread > (as it potentially was for the instance in DelayedTaskManager). > > Maybe something like this on the constructor instead: > > // Constructs a SchedulerParallelTaskRunner which can be used to post tasks so > long as |executor| is alive. > // TODO(robliao): Find a concrete way to manage |executor|'s memory. > SchedulerParallelTaskRunner(... Done. https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:161: // Verify that we are passed |task|'s delayed run time (i.e. |task| is ready On 2016/04/14 19:34:16, gab wrote: > s/passed/past/ > > rm "we", e.g.: > > // Confirm that |task| is ready to run (its delayed run time is either null or > in the past). Done (removed the DCHECK here, kept in in utils.cc). https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ta... File base/task_scheduler/task_traits.cc (right): https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ta... base/task_scheduler/task_traits.cc:38: return with_file_io_ == other.with_file_io_ && priority_ == other.priority_ && On 2016/04/14 20:32:05, robliao wrote: > On 2016/04/14 19:34:16, gab wrote: > > Could we do something like static_assert(sizeof(TaskTraits) == whatever-it-is, > > "TaskTraits size changed, verify that operator=='s implementation is still > > valid.") > > ? > > > > This probably would not work across compiler toolchains and all with a fixed > > value but I'd really like to find a way to make sure this is updated when new > > fields are added to the struct... > > > > @Rob: ideas here? > > > > Or we make this a test only interface by moving the operator== outside the > class > > into the anonymous namespace of the test that needs it to be defined. > > Adding discussion notes from chat: > > This appears to work: > static_assert(offsetof(TaskTraits, shutdown_behavior_) + > sizeof(TaskTraits::shutdown_behavior_) == sizeof(TaskTraits), "Size Changed. > Update operator=="); > > You designate the last expected value and check that your size and size from the > expected value are the same. > > But if we move operator== outside, we'll need to friend function it. Done. https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ut... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ut... base/task_scheduler/utils.cc:54: DCHECK_LE(task->delayed_run_time, TimeTicks::Now()); On 2016/04/14 19:34:16, gab wrote: > Since the same DCHECK is already in PostTaskWithSequence can we keep only one of > them and get the same coverage? I'm guessing this one has even more coverage per > being in the helper method? Done. Removed the DCHECK from STP::PostTaskWithSequence. https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ut... File base/task_scheduler/utils_unittest.cc (right): https://codereview.chromium.org/1708773002/diff/650001/base/task_scheduler/ut... base/task_scheduler/utils_unittest.cc:68: } On 2016/04/14 19:34:16, gab wrote: > Explicitly VerifyAndClear strict mock at the end? It wasn't initially clear to > me how the lack of a call was being verified. Adding VerifyAndClear isn't useful since it's already called when the mock object is destroyed. Added a comment instead.
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, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1708773002/#ps670001 (title: "CR danakj #43")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1708773002/670001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708773002/670001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
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/1708773002/#ps690001 (title: "remove static_assert")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1708773002/690001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708773002/690001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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/1708773002/#ps710001 (title: "fix windows build error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1708773002/710001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708773002/710001
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [7] SchedulerThreadPool A SchedulerThreadPool manages a pool of threads that run Tasks. It provides a CreateTaskRunnerWithTraits() method to create TaskRunners whose PostTask invocations will result in scheduling Tasks on these threads. For now, only the PARALLEL ExecutionMode is supported. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [7] SchedulerThreadPool A SchedulerThreadPool manages a pool of threads that run Tasks. It provides a CreateTaskRunnerWithTraits() method to create TaskRunners whose PostTask invocations will result in scheduling Tasks on these threads. For now, only the PARALLEL ExecutionMode is supported. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #37 (id:710001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [7] SchedulerThreadPool A SchedulerThreadPool manages a pool of threads that run Tasks. It provides a CreateTaskRunnerWithTraits() method to create TaskRunners whose PostTask invocations will result in scheduling Tasks on these threads. For now, only the PARALLEL ExecutionMode is supported. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [7] SchedulerThreadPool A SchedulerThreadPool manages a pool of threads that run Tasks. It provides a CreateTaskRunnerWithTraits() method to create TaskRunners whose PostTask invocations will result in scheduling Tasks on these threads. For now, only the PARALLEL ExecutionMode is supported. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 Committed: https://crrev.com/7422e74e0ca8ff3bada133fce426c59a8094040c Cr-Commit-Position: refs/heads/master@{#387593} ==========
Message was sent while issue was closed.
Patchset 37 (id:??) landed as https://crrev.com/7422e74e0ca8ff3bada133fce426c59a8094040c Cr-Commit-Position: refs/heads/master@{#387593} |
