Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1876363004:
TaskScheduler [11] Support ExecutionMode::SINGLE_THREADED. (Closed)
DescriptionTaskScheduler [11] Support ExecutionMode::SINGLE_THREADED.
Support ExecutionMode::SINGLE_THREADED in
SchedulerThreadPool::CreateTaskRunnerWithTraits. All tasks posted
to a SINGLE_THREADED TaskRunner run on the same thread in posting
order.
BUG=553459
Committed: https://crrev.com/1e7a257a501d2d97fe6550fa023ca2998563c495
Cr-Commit-Position: refs/heads/master@{#390394}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : self review (remove unused headers) #
Total comments: 16
Patch Set 4 : CR gab #4 (comment and naming nits) #Patch Set 5 : self review #Patch Set 6 : self review #
Total comments: 2
Patch Set 7 : CR robliao (single_thread_priority_queue -> alternate_priority_queue) #Patch Set 8 : rebase #Patch Set 9 : rebase #Patch Set 10 : rebase #
Total comments: 6
Patch Set 11 : all post tasks go through SchedulerThreadPool #
Total comments: 2
Patch Set 12 : rebase #Patch Set 13 : rebase #
Total comments: 36
Patch Set 14 : CR gab #20 #
Total comments: 14
Patch Set 15 : CR gab #23 #
Total comments: 8
Patch Set 16 : CR gab #27 (formatting) #
Total comments: 12
Patch Set 17 : always define ContainsWorkerThread, remove unused include #
Total comments: 7
Patch Set 18 : CR danakj #39 #Patch Set 19 : self review #
Total comments: 2
Patch Set 20 : add #include <algorithm> #Messages
Total messages: 62 (15 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
gab@/robliao@: Can you review this CL? Thanks! Note that SchedulerThreadPool::CreateTaskRunnerWithTraits still doesn't support ExecutionMode::SINGLE_THREAD. Before we support it, we have to determine how a thread pool will choose for which worker thread it returns a single-threaded TaskRunner.
gab@/robliao@: Can you review this CL? Thanks! Note that SchedulerThreadPool::CreateTaskRunnerWithTraits still doesn't support ExecutionMode::SINGLE_THREAD. Before we support it, we have to determine how a thread pool will choose for which worker thread it returns a single-threaded TaskRunner.
lgtm % comment and naming nits, thanks! https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:32: SchedulerTaskExecutor* executor, TODO on memory management for this one as well for the sake of keeping track of them. https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:126: // thread has already been woken up to run it. How about s/run/process/ (at first I thought "it" referred to the task instead of the sequence and I think "process" makes this clearer, i.e. process the sequence by running its tasks) https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:128: WakeUp(); Add a TODO to check for removal from idle stack (as you seem to be doing that in a follow-up CL) https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:54: bool* is_single_threaded_sequence) = 0; I don't think the Delegate needs to know that this PQ is "single_threaded". How about "PQ* alternate_priority_queue" and "bool *alternate_priority_queue_used" with comment above changed to: // |alternate_priority_queue| will be considered as an alternate source of work. |alternate_priority_queue_used| is set to true if the returned Sequence was selected from it. https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:56: // Called when a non-single-threaded Sequence returned by GetWork() isn't Idem: re-word without enforcing "single-threaded" https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:65: // thread is allowed to have an active Transaction when its creates a s/its creates/it creates/ but also "it" doesn't create the Transaction in practice, GetWork() does, so it's more "|predecessor_priority_queue| is a PriorityQueue whose transactions may overlap this thread's PriorityQueue's transactions" https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:86: const size_t kNumSequencesPerTest = 150; s/const/constexpr/ (see chromium-dev about new C++11 feature for Chromium -- tl;dr; prefer constexpr now for all compile-time constants) https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:294: Unretained(this), num_posted_tasks_++, task_runner); Only need to lock to get index from |num_posted_tasks_++|, right? Could we instead build the closure outside the lock to highlight that?
gab@: Done. robliao@: Can you review this CL? https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:32: SchedulerTaskExecutor* executor, On 2016/04/18 18:04:02, gab wrote: > TODO on memory management for this one as well for the sake of keeping track of > them. Done. https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:126: // thread has already been woken up to run it. On 2016/04/18 18:04:02, gab wrote: > How about s/run/process/ (at first I thought "it" referred to the task instead > of the sequence and I think "process" makes this clearer, i.e. process the > sequence by running its tasks) Done. https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:128: WakeUp(); On 2016/04/18 18:04:02, gab wrote: > Add a TODO to check for removal from idle stack (as you seem to be doing that in > a follow-up CL) Done. https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:54: bool* is_single_threaded_sequence) = 0; On 2016/04/18 18:04:02, gab wrote: > I don't think the Delegate needs to know that this PQ is "single_threaded". > > How about "PQ* alternate_priority_queue" and "bool > *alternate_priority_queue_used" with comment above changed to: > > // |alternate_priority_queue| will be considered as an alternate source of work. > |alternate_priority_queue_used| is set to true if the returned Sequence was > selected from it. Done. https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:56: // Called when a non-single-threaded Sequence returned by GetWork() isn't On 2016/04/18 18:04:02, gab wrote: > Idem: re-word without enforcing "single-threaded" Done. https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:65: // thread is allowed to have an active Transaction when its creates a On 2016/04/18 18:04:02, gab wrote: > s/its creates/it creates/ > > but also "it" doesn't create the Transaction in practice, GetWork() does, so > it's more "|predecessor_priority_queue| is a PriorityQueue whose transactions > may overlap this thread's PriorityQueue's transactions" Done. https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:86: const size_t kNumSequencesPerTest = 150; On 2016/04/18 18:04:02, gab wrote: > s/const/constexpr/ (see chromium-dev about new C++11 feature for Chromium -- > tl;dr; prefer constexpr now for all compile-time constants) Done. https://codereview.chromium.org/1876363004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:294: Unretained(this), num_posted_tasks_++, task_runner); On 2016/04/18 18:04:02, gab wrote: > Only need to lock to get index from |num_posted_tasks_++|, right? Could we > instead build the closure outside the lock to highlight that? Done.
https://codereview.chromium.org/1876363004/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1876363004/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:129: PriorityQueue* single_threaded_priority_queue, single_thread_priority_queue -> alternate_priority_queue to match the signature from SchedulerWorkerThread::Delegate. Same with is_single_threaded_sequence.
robliao@: Can you take another look? Thanks. https://codereview.chromium.org/1876363004/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1876363004/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:129: PriorityQueue* single_threaded_priority_queue, On 2016/04/18 19:53:51, robliao wrote: > single_thread_priority_queue -> alternate_priority_queue to match the signature > from SchedulerWorkerThread::Delegate. > > Same with is_single_threaded_sequence. Done.
lgtm
fdoray@chromium.org changed reviewers: + danakj@chromium.org
danakj@: Can you review this CL? (Delayed task manager https://codereview.chromium.org/1806473002/ should be reviewed first) Thanks!
Description was changed from ========== TaskScheduler [10] SingleThreadTaskRunner on SchedulerWorkerThread. Add SchedulerWorkerThread::CreateTaskRunnerWithTraits(). This method returns a SingleThreadTaskRunner whose PostTask invocations will result on scheduling Tasks with the specified Traits on the SchedulerWorkerThread on which it's called. BUG=553459 ========== to ========== TaskScheduler [10] SingleThreadTaskRunner on SchedulerWorkerThread. Add SchedulerWorkerThread::CreateTaskRunnerWithTraits(). This method returns a SingleThreadTaskRunner whose PostTask invocations will result on scheduling Tasks with the specified Traits on the SchedulerWorkerThread. BUG=553459 ==========
danakj@: Can you review this CL? Thanks.
Hey, few questions for you bout the relationship we're building between WorkerThread and ThreadPool. https://codereview.chromium.org/1876363004/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1876363004/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:252: PriorityQueue* alternate_priority_queue, I would not add these arguments until you're going to use them, it's hard to review them now. https://codereview.chromium.org/1876363004/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/1876363004/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:51: delay.is_zero() ? TimeTicks() : TimeTicks::Now() + delay)), nit: maybe this conversion from delta to ticks could be in common code (inside PostTaskToExecutor, or whatever it calls)? https://codereview.chromium.org/1876363004/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:171: this, &single_threaded_priority_queue_, &is_single_threaded_sequence); Or maybe you can explain some of the design choices here. 1. I'm not clear why SchedulerWorkerThread is making a task runner instead of SchedulerThreadPool making one that runs tasks on a given fixed worker thread. It would just bind the SchedulerWorkerThread into the task runner in some way? 2. PostTaskWithSequence looks very similar to SchedulerThreadPool's. What would be needed to make SchedulerThreadPool the executor for single thread tasks and share that method? A thread* argument to PostTaskWithSequence that's null if it's not bound to a thread? There's no intention to use these things on a worker thread that *isn't* inside a SchedulerThreadPool is there? 3. I guess the goal here is to try choose between this thread's queue and the global pool queue? It feels a bit unclear the way it's being done here, passing a pointer to "maybe use this". What about delegate_->CheckShouldRunTask() or equivalent with the top of our priority queue, and if it's not true, then we GetWork() for something else to do? Or the delegate_ could be responsible for looking at the thread's pool to choose work and enqueue into it? As it is now, the delegate_ is only for things related to the global thread pool, which we could preserve, it'd be nice to have the |delegate_| name reflect that somehow so it's less hidden then? Or maybe the methods could reflect that like GetWorkFromThreadPool and EnqueueSequenceInThreadPool or something?
Description was changed from ========== TaskScheduler [10] SingleThreadTaskRunner on SchedulerWorkerThread. Add SchedulerWorkerThread::CreateTaskRunnerWithTraits(). This method returns a SingleThreadTaskRunner whose PostTask invocations will result on scheduling Tasks with the specified Traits on the SchedulerWorkerThread. BUG=553459 ========== to ========== TaskScheduler [11] SingleThreadTaskRunner on SchedulerWorkerThread. Add SchedulerWorkerThread::CreateTaskRunnerWithTraits(). This method returns a SingleThreadTaskRunner whose PostTask invocations will result on scheduling Tasks with the specified Traits on the SchedulerWorkerThread. BUG=553459 ==========
Description was changed from ========== TaskScheduler [11] SingleThreadTaskRunner on SchedulerWorkerThread. Add SchedulerWorkerThread::CreateTaskRunnerWithTraits(). This method returns a SingleThreadTaskRunner whose PostTask invocations will result on scheduling Tasks with the specified Traits on the SchedulerWorkerThread. BUG=553459 ========== to ========== TaskScheduler [11] Support ExecutionMode::SINGLE_THREADED. Support ExecutionMode::SINGLE_THREADED in SchedulerThreadPool::CreateTaskRunnerWithTraits. All tasks posted to a SINGLE_THREADED TaskRunner run on the same thread, in posting order. BUG=553459 ==========
Description was changed from ========== TaskScheduler [11] Support ExecutionMode::SINGLE_THREADED. Support ExecutionMode::SINGLE_THREADED in SchedulerThreadPool::CreateTaskRunnerWithTraits. All tasks posted to a SINGLE_THREADED TaskRunner run on the same thread, in posting order. BUG=553459 ========== to ========== TaskScheduler [11] Support ExecutionMode::SINGLE_THREADED. Support ExecutionMode::SINGLE_THREADED in SchedulerThreadPool::CreateTaskRunnerWithTraits. All tasks posted to a SINGLE_THREADED TaskRunner run on the same thread in posting order. BUG=553459 ==========
Dana's review gave me inspiration for a new way to support single-threaded tasks. Patch set 11 is completely different from patch set 10. robliao@/gab@/danakj@: Can you re-review this CL? Thanks. https://codereview.chromium.org/1876363004/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1876363004/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:252: PriorityQueue* alternate_priority_queue, On 2016/04/20 23:16:16, danakj wrote: > I would not add these arguments until you're going to use them, it's hard to > review them now. n/a with the latest patch set https://codereview.chromium.org/1876363004/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/1876363004/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:51: delay.is_zero() ? TimeTicks() : TimeTicks::Now() + delay)), On 2016/04/20 23:16:16, danakj wrote: > nit: maybe this conversion from delta to ticks could be in common code (inside > PostTaskToExecutor, or whatever it calls)? We decided to construct the Task outside of PostTaskToExecutor to avoid passing multiple arguments that are just forwarded to the constructor of Task. I moved this logic to the constructor of Task: https://codereview.chromium.org/1906813003/ https://codereview.chromium.org/1876363004/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:171: this, &single_threaded_priority_queue_, &is_single_threaded_sequence); On 2016/04/20 23:16:16, danakj wrote: > Or maybe you can explain some of the design choices here. > > 1. I'm not clear why SchedulerWorkerThread is making a task runner instead of > SchedulerThreadPool making one that runs tasks on a given fixed worker thread. > It would just bind the SchedulerWorkerThread into the task runner in some way? > > 2. PostTaskWithSequence looks very similar to SchedulerThreadPool's. What would > be needed to make SchedulerThreadPool the executor for single thread tasks and > share that method? A thread* argument to PostTaskWithSequence that's null if > it's not bound to a thread? > > There's no intention to use these things on a worker thread that *isn't* inside > a SchedulerThreadPool is there? > > 3. I guess the goal here is to try choose between this thread's queue and the > global pool queue? It feels a bit unclear the way it's being done here, passing > a pointer to "maybe use this". > > What about delegate_->CheckShouldRunTask() or equivalent with the top of our > priority queue, and if it's not true, then we GetWork() for something else to > do? > > Or the delegate_ could be responsible for looking at the thread's pool to choose > work and enqueue into it? As it is now, the delegate_ is only for things related > to the global thread pool, which we could preserve, it'd be nice to have the > |delegate_| name reflect that somehow so it's less hidden then? Or maybe the > methods could reflect that like GetWorkFromThreadPool and > EnqueueSequenceInThreadPool or something? 1. SchedulerThreadPool could create the TaskRunner. 2. The methods aren't so similar. STP::PostTaskWithSequence posts a task to a shared PQ and calls STP::WakeUpOneThread while SWT::PostTaskWithSequence posts a task to a single-threaded PQ and calls SWT::WakeUp. If we decide to post single-threaded tasks via STP::PostTaskWithSequence, STP would need access to the single-threaded PQ of a SWT. We could add a public accessor on SWT or, even better, STP could own single-threaded PQs in an std::unordered_map<SWT*, unique_ptr<PQ>>. Pseudo-code: void STP::PostTaskWithSequence( std::unique_ptr<Task> task, scoped_refptr<Sequence> sequence, SchedulerWorkerThread* worker_thread) { PriorityQueue* priority_queue = worker_thread ? single_threaded_priority_queues_[worker_thread].get() : &shared_priority_queue_; const bool sequence_was_empty = AddTaskToSequenceAndPriorityQueue( std::move(task), std::move(sequence), priority_queue); if (sequence_was_empty) { if (worker_thread) worker_thread->WakeUp(); else WakeUpOneThread(); } } There is no intention to use a SchedulerWorkerThread outside of a SchedulerThreadPool. 3. If STP owns single-threaded PQs, SWT no longer need to pass a pointer to its single-threaded PQ to its delegate. STP::SWTDelegateImpl::GetWork can decide what Sequence to return using the members of STP only. CheckShouldRunTask(): That method would take a single-threaded PQ as argument and return true if the priority of the Sequence at the top of this PQ is higher than the priority of the Sequence at the top of the shared PQ? The problem with that is that another worker thread could pop a Sequence between the call to CheckShouldRunTask() and GetWork(). We prefer to have a method which atomically checks which PQ has the Sequence with the highest priority, pops the Sequence and returns it. GetWorkFromThreadPool/EnqueueSequenceInThreadPool: We prefer if SWT doesn't know that it is part of a thread pool.
Dana's review gave me inspiration for a new way to support single-threaded tasks. Patch set 11 is completely different from patch set 10. robliao@/gab@/danakj@: Can you re-review this CL? Thanks. This CL now depends on: - https://codereview.chromium.org/1892033003/ SchedulerUniqueStack - https://codereview.chromium.org/1906083002/ Remove base/task_scheduler/utils.h/.cc - https://codereview.chromium.org/1903103007/ Make SchedulerWorkerThread own its delegate. - https://codereview.chromium.org/1906813003/ TimeDelta instead of TimeTicks in Task's constructor.
[rebased] robliao@/gab@/danakj@: Can you re-review this CL? Thanks.
Overall approach lg, a few things below. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:41: // |worker_thread| is optional. Given the above comment says it will be "passed to |thread_pool|" it's weird to now say it's optional. How about "|worker_thread| may be nullptr"? https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:48: SchedulerWorkerThread* worker_thread, I'm not a fan of having a |worker_thread| parameter here but I guess that's the price to pay for deciding not to use Callbacks to Bind all the required post-task state in it. PS: Nothing to do here merely noting my observation... https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:48: // |task| will run on |worker_thread| if specified. The scheduler's Add a note that SchedulerThreadPool has to be the owner of SchedulerWorkerThread (and then DCHECK that this is true in the impl)? https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:233: // SingleThreadTaskRunner. Expand this TODO to mention that this currently assumes all SchedulerWorkerThread are alive which we said wouldn't always be true (could expand dynamically instead of creating |max_threads| right away or could dynamically shrink and keep unbacked SchedulerWorkerThread objects) https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:396: single_threaded_transaction.reset(); Add an inner-scope here for the above code instead of explicit reset calls? https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:452: std::move(single_threaded_priority_queue); The more I read this CL the less I like this cyclic dependency... Could we perhaps instead expose the Delegate from SWThread and cast it to get its PQ directly (would also avoid using a map to store and find something we already have). https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.h:117: std::unordered_map<SchedulerWorkerThread*, std::unique_ptr<PriorityQueue>> TODO about memory management (if we keep the map, see other comment below) https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl_unittest.cc (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl_unittest.cc:29: #include "base/threading/thread_checker_impl.h" Unused? https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl_unittest.cc:320: ::testing::Values(ExecutionMode::SINGLE_THREADED)); Seeing no other modification then this to this test (i.e. no if (GetParam() == SINGLE_THREADED) tells me we're not really testing the single-threaded stuff, i.e. the impl could return a SEQUENCED task runner and this test would be happy.. can we come up with a test that covers the underlying impl further? https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:94: void OnMainEntry(SchedulerWorkerThread* worker_thread) override { EXPECT_EQ on |worker_thread|? Or at least EXPECT_TRUE(worker_thread)?
https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:48: SchedulerWorkerThread* worker_thread, On 2016/04/25 21:00:20, gab wrote: > I'm not a fan of having a |worker_thread| parameter here but I guess that's the > price to pay for deciding not to use Callbacks to Bind all the required > post-task state in it. > > PS: Nothing to do here merely noting my observation... Alternatively we could have two delayed tasks methods. AddDelayedTask and AddSingleThreadedDelayedTask. I'm actually leaning towards this since it avoids the nullptr fun. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:63: struct DelayedTask; I wonder if at some point if it makes sense to expose DelayedTask. AddDelayedTask basically passes the arguments to the DelayedTask constructor. The delayed task index could be managed separately as a static. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:43: virtual bool PostTaskWithSequence(std::unique_ptr<Task> task, Similarly, should we instead have PostSingleThreadedTaskWithSequence?
gab@/robliao@/danakj@: Can you take another look? Thanks. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:41: // |worker_thread| is optional. On 2016/04/25 21:00:20, gab wrote: > Given the above comment says it will be "passed to |thread_pool|" it's weird to > now say it's optional. > > How about "|worker_thread| may be nullptr"? Done. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:48: SchedulerWorkerThread* worker_thread, On 2016/04/25 21:42:38, robliao wrote: > On 2016/04/25 21:00:20, gab wrote: > > I'm not a fan of having a |worker_thread| parameter here but I guess that's > the > > price to pay for deciding not to use Callbacks to Bind all the required > > post-task state in it. > > > > PS: Nothing to do here merely noting my observation... > > Alternatively we could have two delayed tasks methods. > AddDelayedTask and AddSingleThreadedDelayedTask. I'm actually leaning towards > this since it avoids the nullptr fun. The 3 solutions are: 1. Keep this as-is. 2. Add a new method AddSingleThreadedDelayedTask. 3. Instead of passing multiple arguments to AddDelayedTask, expose DelayedTask and pass a DelayedTask. 1. and 3. are good solutions. I'm less excited about 2. because: - SchedulerThreadPool::PostTaskWithSequence will have to behave differently for shared/single-threaded Tasks (or we'll need to add PostSingleThreadedTaskWithSequence). - We'll need an helper function for the code that is common between AddDelayedTask/AddSingleThreadedDelayedTask (unless |worker_thread| stays optional on AddDelayedTask?). An helper function makes it harder to understand the post task flow. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:63: struct DelayedTask; On 2016/04/25 21:42:38, robliao wrote: > I wonder if at some point if it makes sense to expose DelayedTask. > AddDelayedTask basically passes the arguments to the DelayedTask constructor. > The delayed task index could be managed separately as a static. see previous comment https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:43: virtual bool PostTaskWithSequence(std::unique_ptr<Task> task, On 2016/04/25 21:42:38, robliao wrote: > Similarly, should we instead have PostSingleThreadedTaskWithSequence? danakj@: What is your take on this? I think that you like having a single post task flow. I'd like to avoid duplicating the code in PostTaskWithSequence. That means that if we add PostSingleThreadedTaskWithSequence, we need an helper function which will make it hard to understand the post task flow. If we add PostSingleThreadedTaskWithSequence, we probably also need to add PostSingleThreadedTaskWithSequenceNow. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:48: // |task| will run on |worker_thread| if specified. The scheduler's On 2016/04/25 21:00:20, gab wrote: > Add a note that SchedulerThreadPool has to be the owner of SchedulerWorkerThread > (and then DCHECK that this is true in the impl)? Done. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:233: // SingleThreadTaskRunner. On 2016/04/25 21:00:20, gab wrote: > Expand this TODO to mention that this currently assumes all > SchedulerWorkerThread are alive which we said wouldn't always be true (could > expand dynamically instead of creating |max_threads| right away or could > dynamically shrink and keep unbacked SchedulerWorkerThread objects) This code doesn't assume that all SchedulerWorkerThread are alive. I would like if we could create a TaskRunner from any SchedulerWorkerThread, regardless of whether it is backed by a real thread. The first WakeUp() will create the underlying thread if required. Does that work? https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:452: std::move(single_threaded_priority_queue); On 2016/04/25 21:00:20, gab wrote: > The more I read this CL the less I like this cyclic dependency... > > Could we perhaps instead expose the Delegate from SWThread and cast it to get > its PQ directly (would also avoid using a map to store and find something we > already have). Done. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.h:117: std::unordered_map<SchedulerWorkerThread*, std::unique_ptr<PriorityQueue>> On 2016/04/25 21:00:20, gab wrote: > TODO about memory management (if we keep the map, see other comment below) Removed this map. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl_unittest.cc (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl_unittest.cc:29: #include "base/threading/thread_checker_impl.h" On 2016/04/25 21:00:20, gab wrote: > Unused? Done. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl_unittest.cc:320: ::testing::Values(ExecutionMode::SINGLE_THREADED)); On 2016/04/25 21:00:20, gab wrote: > Seeing no other modification then this to this test (i.e. no if (GetParam() == > SINGLE_THREADED) tells me we're not really testing the single-threaded stuff, > i.e. the impl could return a SEQUENCED task runner and this test would be > happy.. can we come up with a test that covers the underlying impl further? TestTaskFactory verifies that all tasks run on the same thread when it is instantiated with SINGLE_THREADED. API: https://codereview.chromium.org/1911493002/diff/80001/base/task_scheduler/tes... Code: https://codereview.chromium.org/1911493002/diff/80001/base/task_scheduler/tes... https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:94: void OnMainEntry(SchedulerWorkerThread* worker_thread) override { On 2016/04/25 21:00:20, gab wrote: > EXPECT_EQ on |worker_thread|? Or at least EXPECT_TRUE(worker_thread)? Done.
https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:48: SchedulerWorkerThread* worker_thread, On 2016/04/25 22:34:44, fdoray wrote: > On 2016/04/25 21:42:38, robliao wrote: > > On 2016/04/25 21:00:20, gab wrote: > > > I'm not a fan of having a |worker_thread| parameter here but I guess that's > > the > > > price to pay for deciding not to use Callbacks to Bind all the required > > > post-task state in it. > > > > > > PS: Nothing to do here merely noting my observation... > > > > Alternatively we could have two delayed tasks methods. > > AddDelayedTask and AddSingleThreadedDelayedTask. I'm actually leaning towards > > this since it avoids the nullptr fun. > > The 3 solutions are: > 1. Keep this as-is. > 2. Add a new method AddSingleThreadedDelayedTask. > 3. Instead of passing multiple arguments to AddDelayedTask, expose DelayedTask > and pass a DelayedTask. > > 1. and 3. are good solutions. I'm less excited about 2. because: > - SchedulerThreadPool::PostTaskWithSequence will have to behave differently for > shared/single-threaded Tasks (or we'll need to add > PostSingleThreadedTaskWithSequence). > - We'll need an helper function for the code that is common between > AddDelayedTask/AddSingleThreadedDelayedTask (unless |worker_thread| stays > optional on AddDelayedTask?). An helper function makes it harder to understand > the post task flow. Meh, not sure about 3. either as DelayedTask's constructor will still need the optionally nullptr argument and so will SchedulerThreadPool::PosttaskWithSequence so we're not gaining much. As I said in my original comment, not a fan of 1., but I think it's the lesser evil (and not something likely to haunt us later given this is an internal API and not a huge readability burden). https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:43: virtual bool PostTaskWithSequence(std::unique_ptr<Task> task, On 2016/04/25 22:34:44, fdoray wrote: > On 2016/04/25 21:42:38, robliao wrote: > > Similarly, should we instead have PostSingleThreadedTaskWithSequence? > > danakj@: What is your take on this? I think that you like having a single post > task flow. +1 to single post task flow. > > I'd like to avoid duplicating the code in PostTaskWithSequence. That means that > if we add PostSingleThreadedTaskWithSequence, we need an helper function which > will make it hard to understand the post task flow. > > If we add PostSingleThreadedTaskWithSequence, we probably also need to add > PostSingleThreadedTaskWithSequenceNow. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:233: // SingleThreadTaskRunner. On 2016/04/25 22:34:44, fdoray wrote: > On 2016/04/25 21:00:20, gab wrote: > > Expand this TODO to mention that this currently assumes all > > SchedulerWorkerThread are alive which we said wouldn't always be true (could > > expand dynamically instead of creating |max_threads| right away or could > > dynamically shrink and keep unbacked SchedulerWorkerThread objects) > > This code doesn't assume that all SchedulerWorkerThread are alive. I would like > if we could create a TaskRunner from any SchedulerWorkerThread, regardless of > whether it is backed by a real thread. The first WakeUp() will create the > underlying thread if required. Does that work? This makes for a heavy WakeUp(), our design doc stipulates that there is always one thread kept idle but ready to fire (i.e. at least one idle shared thread and no thread assigned to single-threaded work can go away anyways). https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl_unittest.cc (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl_unittest.cc:320: ::testing::Values(ExecutionMode::SINGLE_THREADED)); On 2016/04/25 22:34:44, fdoray wrote: > On 2016/04/25 21:00:20, gab wrote: > > Seeing no other modification then this to this test (i.e. no if (GetParam() == > > SINGLE_THREADED) tells me we're not really testing the single-threaded stuff, > > i.e. the impl could return a SEQUENCED task runner and this test would be > > happy.. can we come up with a test that covers the underlying impl further? > > TestTaskFactory verifies that all tasks run on the same thread when it is > instantiated with SINGLE_THREADED. > > API: > https://codereview.chromium.org/1911493002/diff/80001/base/task_scheduler/tes... > Code: > https://codereview.chromium.org/1911493002/diff/80001/base/task_scheduler/tes... Ah I had missed that the extracted TestTaskFactory already had logic for SINGLE_THREADED and was surprised not to see some in this CL, all good then :-)! https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:41: // |worker_thread| may be nullptr. Also add a comment here that |worker_thread| must be owner by |thread_pool|? https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:42: // |worker_thread| if specified (must be owned by this thread pool). Returns As-is it's unclear whether the comments in parens refers to |task| or |worker_thread| IMO. How about: "If |worker_thread| is non-null, |task| will be scheduled to run on it specifically (note: |worker_thread| must be owned by this SchedulerThreadPool); otherwise, |task| will be added to the pending shared work" https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:157: bool ContainsWorkerThread( Encapsulate in #if DCHECK_IS_ON() ? That way a reader will be less skeptical about why this is a linear search. https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:168: reinterpret_cast<SchedulerWorkerThreadDelegateImpl*>( \ Is reinterpret_cast safe here? It probably is because SchedulerWorkerThreadDelegate is pure virtual (i.e. no vtable offset to get to SchedulerWorkerThreadDelegateImpl), but I think we should prefer static_cast anyways, e.g.: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:180: const PriorityQueue* shared_priority_queue); It was confusing to me to see |shared_priority_queue| in this constructor until I read the impl and realized it was merely to take a predecessor dependency on |single_threaded_priority_queue|, can you document this in a comment on this constructor? https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:488: initialize_event_.Signal(); Shall this and the member be DCHECK_IS_ON() only as well? https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:102: outer_->main_entry_worker_thread_ = worker_thread; Why can't this directly compare to |worker_thread_| instead of waiting until TearDown()? Is it because of thread-safety with SetUp()? Feels we could make this work with some synchronization logic, feels weird to have this store state merely to verify it in TearDown()..
gab@/robliao@/danakj@: Can you take another look? Thanks. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:233: // SingleThreadTaskRunner. On 2016/04/26 11:46:27, gab wrote: > On 2016/04/25 22:34:44, fdoray wrote: > > On 2016/04/25 21:00:20, gab wrote: > > > Expand this TODO to mention that this currently assumes all > > > SchedulerWorkerThread are alive which we said wouldn't always be true (could > > > expand dynamically instead of creating |max_threads| right away or could > > > dynamically shrink and keep unbacked SchedulerWorkerThread objects) > > > > This code doesn't assume that all SchedulerWorkerThread are alive. I would > like > > if we could create a TaskRunner from any SchedulerWorkerThread, regardless of > > whether it is backed by a real thread. The first WakeUp() will create the > > underlying thread if required. Does that work? > > This makes for a heavy WakeUp(), our design doc stipulates that there is always > one thread kept idle but ready to fire (i.e. at least one idle shared thread and > no thread assigned to single-threaded work can go away anyways). Added TODO requested in your initial comment. It's hard to find a solution without knowing all the requirements (how many single-threaded TaskRunners do we need? will they all be created during startup?) Just to be sure, if max threads = 4, current number of "real" threads = 1, a SingleThreadTaskRunner is already assigned to the only "real" thread, would we spin up a new "real" thread when a new SINGLE_THREADED TaskRunner is requested? https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:41: // |worker_thread| may be nullptr. On 2016/04/26 11:46:27, gab wrote: > Also add a comment here that |worker_thread| must be owner by |thread_pool|? Done. https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:42: // |worker_thread| if specified (must be owned by this thread pool). Returns On 2016/04/26 11:46:27, gab wrote: > As-is it's unclear whether the comments in parens refers to |task| or > |worker_thread| IMO. How about: > > "If |worker_thread| is non-null, |task| will be scheduled to run on it > specifically (note: |worker_thread| must be owned by this SchedulerThreadPool); > otherwise, |task| will be added to the pending shared work" Done. https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:157: bool ContainsWorkerThread( On 2016/04/26 11:46:27, gab wrote: > Encapsulate in #if DCHECK_IS_ON() ? That way a reader will be less skeptical > about why this is a linear search. Done. https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:168: reinterpret_cast<SchedulerWorkerThreadDelegateImpl*>( \ On 2016/04/26 11:46:27, gab wrote: > Is reinterpret_cast safe here? It probably is because > SchedulerWorkerThreadDelegate is pure virtual (i.e. no vtable offset to get to > SchedulerWorkerThreadDelegateImpl), but I think we should prefer static_cast > anyways, e.g.: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... Done. You're right, static_cast is safer. https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:180: const PriorityQueue* shared_priority_queue); On 2016/04/26 11:46:27, gab wrote: > It was confusing to me to see |shared_priority_queue| in this constructor until > I read the impl and realized it was merely to take a predecessor dependency on > |single_threaded_priority_queue|, can you document this in a comment on this > constructor? Done. https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:488: initialize_event_.Signal(); On 2016/04/26 11:46:27, gab wrote: > Shall this and the member be DCHECK_IS_ON() only as well? Done. https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1876363004/diff/260001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:102: outer_->main_entry_worker_thread_ = worker_thread; On 2016/04/26 11:46:27, gab wrote: > Why can't this directly compare to |worker_thread_| instead of waiting until > TearDown()? Is it because of thread-safety with SetUp()? Feels we could make > this work with some synchronization logic, feels weird to have this store state > merely to verify it in TearDown().. OnMainEntry() may be called before |worker_thread_| is set by SetUp(). I now use an event to wait until |worker_thread_| is set.
https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:48: SchedulerWorkerThread* worker_thread, On 2016/04/26 11:46:27, gab wrote: > On 2016/04/25 22:34:44, fdoray wrote: > > On 2016/04/25 21:42:38, robliao wrote: > > > On 2016/04/25 21:00:20, gab wrote: > > > > I'm not a fan of having a |worker_thread| parameter here but I guess > that's > > > the > > > > price to pay for deciding not to use Callbacks to Bind all the required > > > > post-task state in it. > > > > > > > > PS: Nothing to do here merely noting my observation... > > > > > > Alternatively we could have two delayed tasks methods. > > > AddDelayedTask and AddSingleThreadedDelayedTask. I'm actually leaning > towards > > > this since it avoids the nullptr fun. > > > > The 3 solutions are: > > 1. Keep this as-is. > > 2. Add a new method AddSingleThreadedDelayedTask. > > 3. Instead of passing multiple arguments to AddDelayedTask, expose DelayedTask > > and pass a DelayedTask. > > > > 1. and 3. are good solutions. I'm less excited about 2. because: > > - SchedulerThreadPool::PostTaskWithSequence will have to behave differently > for > > shared/single-threaded Tasks (or we'll need to add > > PostSingleThreadedTaskWithSequence). > > - We'll need an helper function for the code that is common between > > AddDelayedTask/AddSingleThreadedDelayedTask (unless |worker_thread| stays > > optional on AddDelayedTask?). An helper function makes it harder to understand > > the post task flow. > > Meh, not sure about 3. either as DelayedTask's constructor will still need the > optionally nullptr argument and so will > SchedulerThreadPool::PosttaskWithSequence so we're not gaining much. > > As I said in my original comment, not a fan of 1., but I think it's the lesser > evil (and not something likely to haunt us later given this is an internal API > and not a huge readability burden). Exposing DelayedTask's constructor allows you to have a version that does not take a WorkerThread argument. That constructor sets a default of nullptr and then funnels the call to the root constructor that takes all the args. DelayedTask(std::unique_ptr<Task> task, scoped_refptr<Sequence> sequence, SchedulerThreadPool* thread_pool, uint64_t index) : DelayedTask(task, sequence, nullptr, thread_pool, index) {}
Re-lgtm w/ potential follow-up for Rob's request + one comment Thanks! https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:48: SchedulerWorkerThread* worker_thread, On 2016/04/26 16:58:56, robliao wrote: > On 2016/04/26 11:46:27, gab wrote: > > On 2016/04/25 22:34:44, fdoray wrote: > > > On 2016/04/25 21:42:38, robliao wrote: > > > > On 2016/04/25 21:00:20, gab wrote: > > > > > I'm not a fan of having a |worker_thread| parameter here but I guess > > that's > > > > the > > > > > price to pay for deciding not to use Callbacks to Bind all the required > > > > > post-task state in it. > > > > > > > > > > PS: Nothing to do here merely noting my observation... > > > > > > > > Alternatively we could have two delayed tasks methods. > > > > AddDelayedTask and AddSingleThreadedDelayedTask. I'm actually leaning > > towards > > > > this since it avoids the nullptr fun. > > > > > > The 3 solutions are: > > > 1. Keep this as-is. > > > 2. Add a new method AddSingleThreadedDelayedTask. > > > 3. Instead of passing multiple arguments to AddDelayedTask, expose > DelayedTask > > > and pass a DelayedTask. > > > > > > 1. and 3. are good solutions. I'm less excited about 2. because: > > > - SchedulerThreadPool::PostTaskWithSequence will have to behave differently > > for > > > shared/single-threaded Tasks (or we'll need to add > > > PostSingleThreadedTaskWithSequence). > > > - We'll need an helper function for the code that is common between > > > AddDelayedTask/AddSingleThreadedDelayedTask (unless |worker_thread| stays > > > optional on AddDelayedTask?). An helper function makes it harder to > understand > > > the post task flow. > > > > Meh, not sure about 3. either as DelayedTask's constructor will still need the > > optionally nullptr argument and so will > > SchedulerThreadPool::PosttaskWithSequence so we're not gaining much. > > > > As I said in my original comment, not a fan of 1., but I think it's the lesser > > evil (and not something likely to haunt us later given this is an internal API > > and not a huge readability burden). > > Exposing DelayedTask's constructor allows you to have a version that does not > take a WorkerThread argument. > > That constructor sets a default of nullptr and then funnels the call to the root > constructor that takes all the args. > > DelayedTask(std::unique_ptr<Task> task, scoped_refptr<Sequence> sequence, > SchedulerThreadPool* thread_pool, uint64_t index) > : DelayedTask(task, sequence, nullptr, thread_pool, index) {} Good point, I'd be okay with that then. Maybe in a follow-up CL to avoid churn all over this one again? https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:306: #endif // DCHECK_IS_ON() Don't need to surround a DCHECK with DCHECK_IS_ON() (only do so when more code above the DCHECK is setting up its conditions). i.e. prefer #if DCHECK_IS_ON() foo = ... DCHECK_EQ(foo, ...); #endif to #if DCHECK_IS_ON() foo = ... #endif DCHECK_EQ(foo, ...); but when everything fits in the DCHECK don't surround it :-)
https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:399: Occurred to me while merging code for https://codereview.chromium.org/1903133003/ that this is nicer as scoped_refptr<Sequence> sequence; { ... } DCHECK(sequence); (i.e. no empty line here and DCHECK at the end to really highlight that the purpose of this scope is to set |sequence| -- essentially as an inline method)
robliao@/danakj@: Can you take another look? Thanks. https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:48: SchedulerWorkerThread* worker_thread, On 2016/04/26 20:30:58, gab wrote: > On 2016/04/26 16:58:56, robliao wrote: > > On 2016/04/26 11:46:27, gab wrote: > > > On 2016/04/25 22:34:44, fdoray wrote: > > > > On 2016/04/25 21:42:38, robliao wrote: > > > > > On 2016/04/25 21:00:20, gab wrote: > > > > > > I'm not a fan of having a |worker_thread| parameter here but I guess > > > that's > > > > > the > > > > > > price to pay for deciding not to use Callbacks to Bind all the > required > > > > > > post-task state in it. > > > > > > > > > > > > PS: Nothing to do here merely noting my observation... > > > > > > > > > > Alternatively we could have two delayed tasks methods. > > > > > AddDelayedTask and AddSingleThreadedDelayedTask. I'm actually leaning > > > towards > > > > > this since it avoids the nullptr fun. > > > > > > > > The 3 solutions are: > > > > 1. Keep this as-is. > > > > 2. Add a new method AddSingleThreadedDelayedTask. > > > > 3. Instead of passing multiple arguments to AddDelayedTask, expose > > DelayedTask > > > > and pass a DelayedTask. > > > > > > > > 1. and 3. are good solutions. I'm less excited about 2. because: > > > > - SchedulerThreadPool::PostTaskWithSequence will have to behave > differently > > > for > > > > shared/single-threaded Tasks (or we'll need to add > > > > PostSingleThreadedTaskWithSequence). > > > > - We'll need an helper function for the code that is common between > > > > AddDelayedTask/AddSingleThreadedDelayedTask (unless |worker_thread| stays > > > > optional on AddDelayedTask?). An helper function makes it harder to > > understand > > > > the post task flow. > > > > > > Meh, not sure about 3. either as DelayedTask's constructor will still need > the > > > optionally nullptr argument and so will > > > SchedulerThreadPool::PosttaskWithSequence so we're not gaining much. > > > > > > As I said in my original comment, not a fan of 1., but I think it's the > lesser > > > evil (and not something likely to haunt us later given this is an internal > API > > > and not a huge readability burden). > > > > Exposing DelayedTask's constructor allows you to have a version that does not > > take a WorkerThread argument. > > > > That constructor sets a default of nullptr and then funnels the call to the > root > > constructor that takes all the args. > > > > DelayedTask(std::unique_ptr<Task> task, scoped_refptr<Sequence> sequence, > > SchedulerThreadPool* thread_pool, uint64_t index) > > : DelayedTask(task, sequence, nullptr, thread_pool, index) {} > > Good point, I'd be okay with that then. Maybe in a follow-up CL to avoid churn > all over this one again? If we want a single version of SchedulerThreadPoolImpl::PostTaskWithSequence(), we don't need a constructor that does not take a WorkerThread argument. Knowing that, do we still want to expose DelayedTask? I'm not a fan of setting |index| after construction. https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:306: #endif // DCHECK_IS_ON() On 2016/04/26 20:30:58, gab wrote: > Don't need to surround a DCHECK with DCHECK_IS_ON() (only do so when more code > above the DCHECK is setting up its conditions). > > i.e. prefer > #if DCHECK_IS_ON() > foo = ... > DCHECK_EQ(foo, ...); > #endif > to > #if DCHECK_IS_ON() > foo = ... > #endif > DCHECK_EQ(foo, ...); > > but when everything fits in the DCHECK don't surround it :-) The argument of a DCHECK() is referenced even when !DCHECK_IS_ON(). That means that without #if DCHECK_IS_ON() here, ContainsWorkerThread() is referenced in non-DCHECK builds. That doesn't compile because the definition of ContainsWorkerThread() is guarded by #if DCHECK_IS_ON(). https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:399: On 2016/04/26 23:26:13, gab wrote: > Occurred to me while merging code for > https://codereview.chromium.org/1903133003/ that this is nicer as > > scoped_refptr<Sequence> sequence; > { > ... > } > DCHECK(sequence); > > (i.e. no empty line here and DCHECK at the end to really highlight that the > purpose of this scope is to set |sequence| -- essentially as an inline method) Done.
https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:48: SchedulerWorkerThread* worker_thread, On 2016/04/27 16:09:59, fdoray wrote: > On 2016/04/26 20:30:58, gab wrote: > > On 2016/04/26 16:58:56, robliao wrote: > > > On 2016/04/26 11:46:27, gab wrote: > > > > On 2016/04/25 22:34:44, fdoray wrote: > > > > > On 2016/04/25 21:42:38, robliao wrote: > > > > > > On 2016/04/25 21:00:20, gab wrote: > > > > > > > I'm not a fan of having a |worker_thread| parameter here but I guess > > > > that's > > > > > > the > > > > > > > price to pay for deciding not to use Callbacks to Bind all the > > required > > > > > > > post-task state in it. > > > > > > > > > > > > > > PS: Nothing to do here merely noting my observation... > > > > > > > > > > > > Alternatively we could have two delayed tasks methods. > > > > > > AddDelayedTask and AddSingleThreadedDelayedTask. I'm actually leaning > > > > towards > > > > > > this since it avoids the nullptr fun. > > > > > > > > > > The 3 solutions are: > > > > > 1. Keep this as-is. > > > > > 2. Add a new method AddSingleThreadedDelayedTask. > > > > > 3. Instead of passing multiple arguments to AddDelayedTask, expose > > > DelayedTask > > > > > and pass a DelayedTask. > > > > > > > > > > 1. and 3. are good solutions. I'm less excited about 2. because: > > > > > - SchedulerThreadPool::PostTaskWithSequence will have to behave > > differently > > > > for > > > > > shared/single-threaded Tasks (or we'll need to add > > > > > PostSingleThreadedTaskWithSequence). > > > > > - We'll need an helper function for the code that is common between > > > > > AddDelayedTask/AddSingleThreadedDelayedTask (unless |worker_thread| > stays > > > > > optional on AddDelayedTask?). An helper function makes it harder to > > > understand > > > > > the post task flow. > > > > > > > > Meh, not sure about 3. either as DelayedTask's constructor will still need > > the > > > > optionally nullptr argument and so will > > > > SchedulerThreadPool::PosttaskWithSequence so we're not gaining much. > > > > > > > > As I said in my original comment, not a fan of 1., but I think it's the > > lesser > > > > evil (and not something likely to haunt us later given this is an internal > > API > > > > and not a huge readability burden). > > > > > > Exposing DelayedTask's constructor allows you to have a version that does > not > > > take a WorkerThread argument. > > > > > > That constructor sets a default of nullptr and then funnels the call to the > > root > > > constructor that takes all the args. > > > > > > DelayedTask(std::unique_ptr<Task> task, scoped_refptr<Sequence> sequence, > > > SchedulerThreadPool* thread_pool, uint64_t index) > > > : DelayedTask(task, sequence, nullptr, thread_pool, index) {} > > > > Good point, I'd be okay with that then. Maybe in a follow-up CL to avoid churn > > all over this one again? > > If we want a single version of SchedulerThreadPoolImpl::PostTaskWithSequence(), > we don't need a constructor that does not take a WorkerThread argument. Knowing > that, do we still want to expose DelayedTask? I'm not a fan of setting |index| > after construction. The thing that I like to avoid is forcing callers to use nullptr's in APIs. It generally means that there probably should have been a different API surface, especially if the nullptr version is the most common one. With regards to setting the index, we're free to set it here as well. https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:169: static_cast<SchedulerWorkerThreadDelegateImpl*>(worker_thread->delegate()) \ There's enough going on here that I wouldn't mind seeing this as just a regular function and letting the compiler inline it. https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.h (right): https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.h:11: #include <unordered_map> Nit: Remove? https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.h:134: // Signaled when all threads have been created. Worth commented why we only care about this when DCHECK_IS_ON (Worker thread check)
Replies to ongoing discussions https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:48: SchedulerWorkerThread* worker_thread, On 2016/04/27 17:43:45, robliao wrote: > On 2016/04/27 16:09:59, fdoray wrote: > > On 2016/04/26 20:30:58, gab wrote: > > > On 2016/04/26 16:58:56, robliao wrote: > > > > On 2016/04/26 11:46:27, gab wrote: > > > > > On 2016/04/25 22:34:44, fdoray wrote: > > > > > > On 2016/04/25 21:42:38, robliao wrote: > > > > > > > On 2016/04/25 21:00:20, gab wrote: > > > > > > > > I'm not a fan of having a |worker_thread| parameter here but I > guess > > > > > that's > > > > > > > the > > > > > > > > price to pay for deciding not to use Callbacks to Bind all the > > > required > > > > > > > > post-task state in it. > > > > > > > > > > > > > > > > PS: Nothing to do here merely noting my observation... > > > > > > > > > > > > > > Alternatively we could have two delayed tasks methods. > > > > > > > AddDelayedTask and AddSingleThreadedDelayedTask. I'm actually > leaning > > > > > towards > > > > > > > this since it avoids the nullptr fun. > > > > > > > > > > > > The 3 solutions are: > > > > > > 1. Keep this as-is. > > > > > > 2. Add a new method AddSingleThreadedDelayedTask. > > > > > > 3. Instead of passing multiple arguments to AddDelayedTask, expose > > > > DelayedTask > > > > > > and pass a DelayedTask. > > > > > > > > > > > > 1. and 3. are good solutions. I'm less excited about 2. because: > > > > > > - SchedulerThreadPool::PostTaskWithSequence will have to behave > > > differently > > > > > for > > > > > > shared/single-threaded Tasks (or we'll need to add > > > > > > PostSingleThreadedTaskWithSequence). > > > > > > - We'll need an helper function for the code that is common between > > > > > > AddDelayedTask/AddSingleThreadedDelayedTask (unless |worker_thread| > > stays > > > > > > optional on AddDelayedTask?). An helper function makes it harder to > > > > understand > > > > > > the post task flow. > > > > > > > > > > Meh, not sure about 3. either as DelayedTask's constructor will still > need > > > the > > > > > optionally nullptr argument and so will > > > > > SchedulerThreadPool::PosttaskWithSequence so we're not gaining much. > > > > > > > > > > As I said in my original comment, not a fan of 1., but I think it's the > > > lesser > > > > > evil (and not something likely to haunt us later given this is an > internal > > > API > > > > > and not a huge readability burden). > > > > > > > > Exposing DelayedTask's constructor allows you to have a version that does > > not > > > > take a WorkerThread argument. > > > > > > > > That constructor sets a default of nullptr and then funnels the call to > the > > > root > > > > constructor that takes all the args. > > > > > > > > DelayedTask(std::unique_ptr<Task> task, scoped_refptr<Sequence> sequence, > > > > SchedulerThreadPool* thread_pool, uint64_t index) > > > > : DelayedTask(task, sequence, nullptr, thread_pool, index) {} > > > > > > Good point, I'd be okay with that then. Maybe in a follow-up CL to avoid > churn > > > all over this one again? > > > > If we want a single version of > SchedulerThreadPoolImpl::PostTaskWithSequence(), > > we don't need a constructor that does not take a WorkerThread argument. > Knowing > > that, do we still want to expose DelayedTask? I'm not a fan of setting |index| > > after construction. > > The thing that I like to avoid is forcing callers to use nullptr's in APIs. It > generally means that there probably should have been a different API surface, > especially if the nullptr version is the most common one. > > With regards to setting the index, we're free to set it here as well. Either way PostTaskWithSequenceNow() will need to take a potentially null SWorkerThread* so I prefer that DelayedTaskManager have the same API (otherwise we have one API taking a pointer and the other one taking a DelayedTask with the optional field -- which is really a disguised default argument which is banned by the style guide for virtual functions [1] and thus can't be used for PostTaskWithSequenceNow()). [1] https://google.github.io/styleguide/cppguide.html#Default_Arguments https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.h (right): https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.h:134: // Signaled when all threads have been created. On 2016/04/27 17:43:45, robliao wrote: > Worth commented why we only care about this when DCHECK_IS_ON (Worker thread > check) I think the use of DCHECK_IS_ON() is self-documenting as "only needed for DCHECKs" and the exact reason can easily be searched for it in impl if one really wants to know.
https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:306: #endif // DCHECK_IS_ON() On 2016/04/27 16:09:59, fdoray wrote: > On 2016/04/26 20:30:58, gab wrote: > > Don't need to surround a DCHECK with DCHECK_IS_ON() (only do so when more code > > above the DCHECK is setting up its conditions). > > > > i.e. prefer > > #if DCHECK_IS_ON() > > foo = ... > > DCHECK_EQ(foo, ...); > > #endif > > to > > #if DCHECK_IS_ON() > > foo = ... > > #endif > > DCHECK_EQ(foo, ...); > > > > but when everything fits in the DCHECK don't surround it :-) > > The argument of a DCHECK() is referenced even when !DCHECK_IS_ON(). That means > that without #if DCHECK_IS_ON() here, ContainsWorkerThread() is referenced in > non-DCHECK builds. That doesn't compile because the definition of > ContainsWorkerThread() is guarded by #if DCHECK_IS_ON(). Ah good point, still find this ugly though... looks like we could use DLOG_ASSERT() which is truly a blank replacement in non-dcheck builds and otherwise acts as LOG_IF(FATAL, assertion). Ref:https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=600
https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:48: SchedulerWorkerThread* worker_thread, On 2016/04/27 18:11:43, gab wrote: > On 2016/04/27 17:43:45, robliao wrote: > > On 2016/04/27 16:09:59, fdoray wrote: > > > On 2016/04/26 20:30:58, gab wrote: > > > > On 2016/04/26 16:58:56, robliao wrote: > > > > > On 2016/04/26 11:46:27, gab wrote: > > > > > > On 2016/04/25 22:34:44, fdoray wrote: > > > > > > > On 2016/04/25 21:42:38, robliao wrote: > > > > > > > > On 2016/04/25 21:00:20, gab wrote: > > > > > > > > > I'm not a fan of having a |worker_thread| parameter here but I > > guess > > > > > > that's > > > > > > > > the > > > > > > > > > price to pay for deciding not to use Callbacks to Bind all the > > > > required > > > > > > > > > post-task state in it. > > > > > > > > > > > > > > > > > > PS: Nothing to do here merely noting my observation... > > > > > > > > > > > > > > > > Alternatively we could have two delayed tasks methods. > > > > > > > > AddDelayedTask and AddSingleThreadedDelayedTask. I'm actually > > leaning > > > > > > towards > > > > > > > > this since it avoids the nullptr fun. > > > > > > > > > > > > > > The 3 solutions are: > > > > > > > 1. Keep this as-is. > > > > > > > 2. Add a new method AddSingleThreadedDelayedTask. > > > > > > > 3. Instead of passing multiple arguments to AddDelayedTask, expose > > > > > DelayedTask > > > > > > > and pass a DelayedTask. > > > > > > > > > > > > > > 1. and 3. are good solutions. I'm less excited about 2. because: > > > > > > > - SchedulerThreadPool::PostTaskWithSequence will have to behave > > > > differently > > > > > > for > > > > > > > shared/single-threaded Tasks (or we'll need to add > > > > > > > PostSingleThreadedTaskWithSequence). > > > > > > > - We'll need an helper function for the code that is common between > > > > > > > AddDelayedTask/AddSingleThreadedDelayedTask (unless |worker_thread| > > > stays > > > > > > > optional on AddDelayedTask?). An helper function makes it harder to > > > > > understand > > > > > > > the post task flow. > > > > > > > > > > > > Meh, not sure about 3. either as DelayedTask's constructor will still > > need > > > > the > > > > > > optionally nullptr argument and so will > > > > > > SchedulerThreadPool::PosttaskWithSequence so we're not gaining much. > > > > > > > > > > > > As I said in my original comment, not a fan of 1., but I think it's > the > > > > lesser > > > > > > evil (and not something likely to haunt us later given this is an > > internal > > > > API > > > > > > and not a huge readability burden). > > > > > > > > > > Exposing DelayedTask's constructor allows you to have a version that > does > > > not > > > > > take a WorkerThread argument. > > > > > > > > > > That constructor sets a default of nullptr and then funnels the call to > > the > > > > root > > > > > constructor that takes all the args. > > > > > > > > > > DelayedTask(std::unique_ptr<Task> task, scoped_refptr<Sequence> > sequence, > > > > > SchedulerThreadPool* thread_pool, uint64_t index) > > > > > : DelayedTask(task, sequence, nullptr, thread_pool, index) {} > > > > > > > > Good point, I'd be okay with that then. Maybe in a follow-up CL to avoid > > churn > > > > all over this one again? > > > > > > If we want a single version of > > SchedulerThreadPoolImpl::PostTaskWithSequence(), > > > we don't need a constructor that does not take a WorkerThread argument. > > Knowing > > > that, do we still want to expose DelayedTask? I'm not a fan of setting > |index| > > > after construction. > > > > The thing that I like to avoid is forcing callers to use nullptr's in APIs. It > > generally means that there probably should have been a different API surface, > > especially if the nullptr version is the most common one. > > > > With regards to setting the index, we're free to set it here as well. > > Either way PostTaskWithSequenceNow() will need to take a potentially null > SWorkerThread* so I prefer that DelayedTaskManager have the same API (otherwise > we have one API taking a pointer and the other one taking a DelayedTask with the > optional field -- which is really a disguised default argument which is banned > by the style guide for virtual functions [1] and thus can't be used for > PostTaskWithSequenceNow()). > > [1] https://google.github.io/styleguide/cppguide.html#Default_Arguments Yup, agreed on the default argument points. I'm fine with the nullptr as is for now. Just replying to the comment. https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:306: #endif // DCHECK_IS_ON() On 2016/04/27 18:17:47, gab wrote: > On 2016/04/27 16:09:59, fdoray wrote: > > On 2016/04/26 20:30:58, gab wrote: > > > Don't need to surround a DCHECK with DCHECK_IS_ON() (only do so when more > code > > > above the DCHECK is setting up its conditions). > > > > > > i.e. prefer > > > #if DCHECK_IS_ON() > > > foo = ... > > > DCHECK_EQ(foo, ...); > > > #endif > > > to > > > #if DCHECK_IS_ON() > > > foo = ... > > > #endif > > > DCHECK_EQ(foo, ...); > > > > > > but when everything fits in the DCHECK don't surround it :-) > > > > The argument of a DCHECK() is referenced even when !DCHECK_IS_ON(). That means > > that without #if DCHECK_IS_ON() here, ContainsWorkerThread() is referenced in > > non-DCHECK builds. That doesn't compile because the definition of > > ContainsWorkerThread() is guarded by #if DCHECK_IS_ON(). > > Ah good point, still find this ugly though... looks like we could use > DLOG_ASSERT() which is truly a blank replacement in non-dcheck builds and > otherwise acts as LOG_IF(FATAL, assertion). > > Ref:https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=600 Yeah, it was unexpected to me that DCHECK expressions in builds are referenced even when DCHECK_IS_OFF. DLOG_ASSERT might be a suitable replacement. I wonder if it's an issue that DCHECK expressions get compiled even when DCHECK_IS_OFF.
https://codereview.chromium.org/1876363004/diff/200001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1876363004/diff/200001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:83: // |worker_thread| if specified; otherwise it can run on a any thread owned by "on any" https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:306: #endif // DCHECK_IS_ON() On 2016/04/27 18:21:03, robliao wrote: > On 2016/04/27 18:17:47, gab wrote: > > On 2016/04/27 16:09:59, fdoray wrote: > > > On 2016/04/26 20:30:58, gab wrote: > > > > Don't need to surround a DCHECK with DCHECK_IS_ON() (only do so when more > > code > > > > above the DCHECK is setting up its conditions). > > > > > > > > i.e. prefer > > > > #if DCHECK_IS_ON() > > > > foo = ... > > > > DCHECK_EQ(foo, ...); > > > > #endif > > > > to > > > > #if DCHECK_IS_ON() > > > > foo = ... > > > > #endif > > > > DCHECK_EQ(foo, ...); > > > > > > > > but when everything fits in the DCHECK don't surround it :-) > > > > > > The argument of a DCHECK() is referenced even when !DCHECK_IS_ON(). That > means > > > that without #if DCHECK_IS_ON() here, ContainsWorkerThread() is referenced > in > > > non-DCHECK builds. That doesn't compile because the definition of > > > ContainsWorkerThread() is guarded by #if DCHECK_IS_ON(). > > > > Ah good point, still find this ugly though... looks like we could use > > DLOG_ASSERT() which is truly a blank replacement in non-dcheck builds and > > otherwise acts as LOG_IF(FATAL, assertion). > > > > > Ref:https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=600 > > Yeah, it was unexpected to me that DCHECK expressions in builds are referenced > even when DCHECK_IS_OFF. DLOG_ASSERT might be a suitable replacement. > > I wonder if it's an issue that DCHECK expressions get compiled even when > DCHECK_IS_OFF. They compile out but they still reference the variable.
https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/280001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:306: #endif // DCHECK_IS_ON() On 2016/04/27 18:28:43, danakj wrote: > On 2016/04/27 18:21:03, robliao wrote: > > On 2016/04/27 18:17:47, gab wrote: > > > On 2016/04/27 16:09:59, fdoray wrote: > > > > On 2016/04/26 20:30:58, gab wrote: > > > > > Don't need to surround a DCHECK with DCHECK_IS_ON() (only do so when > more > > > code > > > > > above the DCHECK is setting up its conditions). > > > > > > > > > > i.e. prefer > > > > > #if DCHECK_IS_ON() > > > > > foo = ... > > > > > DCHECK_EQ(foo, ...); > > > > > #endif > > > > > to > > > > > #if DCHECK_IS_ON() > > > > > foo = ... > > > > > #endif > > > > > DCHECK_EQ(foo, ...); > > > > > > > > > > but when everything fits in the DCHECK don't surround it :-) > > > > > > > > The argument of a DCHECK() is referenced even when !DCHECK_IS_ON(). That > > means > > > > that without #if DCHECK_IS_ON() here, ContainsWorkerThread() is referenced > > in > > > > non-DCHECK builds. That doesn't compile because the definition of > > > > ContainsWorkerThread() is guarded by #if DCHECK_IS_ON(). > > > > > > Ah good point, still find this ugly though... looks like we could use > > > DLOG_ASSERT() which is truly a blank replacement in non-dcheck builds and > > > otherwise acts as LOG_IF(FATAL, assertion). > > > > > > > > > Ref:https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=600 > > > > Yeah, it was unexpected to me that DCHECK expressions in builds are referenced > > even when DCHECK_IS_OFF. DLOG_ASSERT might be a suitable replacement. > > > > I wonder if it's an issue that DCHECK expressions get compiled even when > > DCHECK_IS_OFF. > > They compile out but they still reference the variable. Right logging.h documents this as being by design for DCHECK to avoid benign "unused variables" warnings. But DLOG_ASSERT also documents being similar to DCHECK minus this property so sounds like what we wound + s/DCHECK_IS_ON/DEBUG_MODE/ for consistency I guess though I think they're equivalent in practice.
robliao@: Can you take another look? https://codereview.chromium.org/1876363004/diff/200001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1876363004/diff/200001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:83: // |worker_thread| if specified; otherwise it can run on a any thread owned by On 2016/04/27 18:28:43, danakj wrote: > "on any" n/a with latest patch set https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1876363004/diff/240001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:48: SchedulerWorkerThread* worker_thread, On 2016/04/27 18:21:03, robliao wrote: > On 2016/04/27 18:11:43, gab wrote: > > On 2016/04/27 17:43:45, robliao wrote: > > > On 2016/04/27 16:09:59, fdoray wrote: > > > > On 2016/04/26 20:30:58, gab wrote: > > > > > On 2016/04/26 16:58:56, robliao wrote: > > > > > > On 2016/04/26 11:46:27, gab wrote: > > > > > > > On 2016/04/25 22:34:44, fdoray wrote: > > > > > > > > On 2016/04/25 21:42:38, robliao wrote: > > > > > > > > > On 2016/04/25 21:00:20, gab wrote: > > > > > > > > > > I'm not a fan of having a |worker_thread| parameter here but I > > > guess > > > > > > > that's > > > > > > > > > the > > > > > > > > > > price to pay for deciding not to use Callbacks to Bind all the > > > > > required > > > > > > > > > > post-task state in it. > > > > > > > > > > > > > > > > > > > > PS: Nothing to do here merely noting my observation... > > > > > > > > > > > > > > > > > > Alternatively we could have two delayed tasks methods. > > > > > > > > > AddDelayedTask and AddSingleThreadedDelayedTask. I'm actually > > > leaning > > > > > > > towards > > > > > > > > > this since it avoids the nullptr fun. > > > > > > > > > > > > > > > > The 3 solutions are: > > > > > > > > 1. Keep this as-is. > > > > > > > > 2. Add a new method AddSingleThreadedDelayedTask. > > > > > > > > 3. Instead of passing multiple arguments to AddDelayedTask, expose > > > > > > DelayedTask > > > > > > > > and pass a DelayedTask. > > > > > > > > > > > > > > > > 1. and 3. are good solutions. I'm less excited about 2. because: > > > > > > > > - SchedulerThreadPool::PostTaskWithSequence will have to behave > > > > > differently > > > > > > > for > > > > > > > > shared/single-threaded Tasks (or we'll need to add > > > > > > > > PostSingleThreadedTaskWithSequence). > > > > > > > > - We'll need an helper function for the code that is common > between > > > > > > > > AddDelayedTask/AddSingleThreadedDelayedTask (unless > |worker_thread| > > > > stays > > > > > > > > optional on AddDelayedTask?). An helper function makes it harder > to > > > > > > understand > > > > > > > > the post task flow. > > > > > > > > > > > > > > Meh, not sure about 3. either as DelayedTask's constructor will > still > > > need > > > > > the > > > > > > > optionally nullptr argument and so will > > > > > > > SchedulerThreadPool::PosttaskWithSequence so we're not gaining much. > > > > > > > > > > > > > > As I said in my original comment, not a fan of 1., but I think it's > > the > > > > > lesser > > > > > > > evil (and not something likely to haunt us later given this is an > > > internal > > > > > API > > > > > > > and not a huge readability burden). > > > > > > > > > > > > Exposing DelayedTask's constructor allows you to have a version that > > does > > > > not > > > > > > take a WorkerThread argument. > > > > > > > > > > > > That constructor sets a default of nullptr and then funnels the call > to > > > the > > > > > root > > > > > > constructor that takes all the args. > > > > > > > > > > > > DelayedTask(std::unique_ptr<Task> task, scoped_refptr<Sequence> > > sequence, > > > > > > SchedulerThreadPool* thread_pool, uint64_t index) > > > > > > : DelayedTask(task, sequence, nullptr, thread_pool, index) {} > > > > > > > > > > Good point, I'd be okay with that then. Maybe in a follow-up CL to avoid > > > churn > > > > > all over this one again? > > > > > > > > If we want a single version of > > > SchedulerThreadPoolImpl::PostTaskWithSequence(), > > > > we don't need a constructor that does not take a WorkerThread argument. > > > Knowing > > > > that, do we still want to expose DelayedTask? I'm not a fan of setting > > |index| > > > > after construction. > > > > > > The thing that I like to avoid is forcing callers to use nullptr's in APIs. > It > > > generally means that there probably should have been a different API > surface, > > > especially if the nullptr version is the most common one. > > > > > > With regards to setting the index, we're free to set it here as well. > > > > Either way PostTaskWithSequenceNow() will need to take a potentially null > > SWorkerThread* so I prefer that DelayedTaskManager have the same API > (otherwise > > we have one API taking a pointer and the other one taking a DelayedTask with > the > > optional field -- which is really a disguised default argument which is banned > > by the style guide for virtual functions [1] and thus can't be used for > > PostTaskWithSequenceNow()). > > > > [1] https://google.github.io/styleguide/cppguide.html#Default_Arguments > > Yup, agreed on the default argument points. I'm fine with the nullptr as is for > now. Just replying to the comment. Keeping this discussion for another CL. https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:169: static_cast<SchedulerWorkerThreadDelegateImpl*>(worker_thread->delegate()) \ On 2016/04/27 17:43:45, robliao wrote: > There's enough going on here that I wouldn't mind seeing this as just a regular > function and letting the compiler inline it. A function in the anonymous namespace couldn't have accessed SchedulerWorkerThreadDelegateImpl because it is private. Anyways, I realized that 1 of the 2 uses of this macro wasn't required. I put the macro's code directly at the only place where it's really required. https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.h (right): https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.h:11: #include <unordered_map> On 2016/04/27 17:43:45, robliao wrote: > Nit: Remove? Done. https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.h:134: // Signaled when all threads have been created. On 2016/04/27 18:11:43, gab wrote: > On 2016/04/27 17:43:45, robliao wrote: > > Worth commented why we only care about this when DCHECK_IS_ON (Worker thread > > check) > > I think the use of DCHECK_IS_ON() is self-documenting as "only needed for > DCHECKs" and the exact reason can easily be searched for it in impl if one > really wants to know. Didn't do anything about this.
lgtm
lgtm, thanks!
danakj@: Can you review this CL? Thanks.
LG! Just a couple questions/comments. https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:160: for (const auto& current_worker_thread : worker_threads) { auto it = std::find_if( worker_threads.begin(), worker_threads.end(), [worker_thread](const std::unique_ptr<SchedulerWorkerThread>& i) { return i.get() == worker_thread; }); return it != worker_threads.end(); also works.. but maybe this is cleaner as is, up to you. thought I'd mention it https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:170: ->single_threaded_priority_queue() I'm wondering if you need the STPriQueue from the delegate, why isn't it a method on the Delegate interface? https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:331: #endif // DCHECK_IS_ON() nit: i dont think you need these ending comments if the if block is very short (~6 lines?) that's what the tooling enforces for namespaces also. https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:422: (!shared_sequence_and_sort_key.is_null() && nit: I don't like combining && and || in one if statement, especially when it's so long. If you can break this into a few bools and use those in the if statement instead that'd be cool. https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:71: SchedulerWorkerThread::Delegate* delegate() { return delegate_.get(); } I also wonder about exposing the delegate here, this seems to be bad for law of demeter. Maybe this could return the priority queue, and the fact that it goes through delegate to get it is an implementation detail?
Reply to one of Dana's question which is my fault :-) https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:71: SchedulerWorkerThread::Delegate* delegate() { return delegate_.get(); } On 2016/04/27 20:38:13, danakj wrote: > I also wonder about exposing the delegate here, this seems to be bad for law of > demeter. Maybe this could return the priority queue, and the fact that it goes > through delegate to get it is an implementation detail? I asked for this. Asking the SchedulerWorkerThread for the priority queue doesn't work because the impl (which is defined and instantiated in SchedulerThreadPoolImpl) has it (not the generic interface) and the SchedulerWorkerThread thus can't reach it (it's not even aware of its existence). Prior to this there was a map in SchedulerThreadPoolImpl to map SchedulerWorkerThreads to their PriorityQueue but I thought it felt silly to do a map lookup to get something we already have hidden in the SchedulerWorkerThread we're using to do the query..!
https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:71: SchedulerWorkerThread::Delegate* delegate() { return delegate_.get(); } On 2016/04/27 20:45:39, gab wrote: > On 2016/04/27 20:38:13, danakj wrote: > > I also wonder about exposing the delegate here, this seems to be bad for law > of > > demeter. Maybe this could return the priority queue, and the fact that it goes > > through delegate to get it is an implementation detail? > > I asked for this. Asking the SchedulerWorkerThread for the priority queue > doesn't work because the impl (which is defined and instantiated in > SchedulerThreadPoolImpl) has it (not the generic interface) and the > SchedulerWorkerThread thus can't reach it (it's not even aware of its > existence). > > Prior to this there was a map in SchedulerThreadPoolImpl to map > SchedulerWorkerThreads to their PriorityQueue but I thought it felt silly to do > a map lookup to get something we already have hidden in the > SchedulerWorkerThread we're using to do the query..! What if Delegate had a GetThreadPriorityQueue?
https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:71: SchedulerWorkerThread::Delegate* delegate() { return delegate_.get(); } On 2016/04/27 20:53:28, danakj wrote: > On 2016/04/27 20:45:39, gab wrote: > > On 2016/04/27 20:38:13, danakj wrote: > > > I also wonder about exposing the delegate here, this seems to be bad for law > > of > > > demeter. Maybe this could return the priority queue, and the fact that it > goes > > > through delegate to get it is an implementation detail? > > > > I asked for this. Asking the SchedulerWorkerThread for the priority queue > > doesn't work because the impl (which is defined and instantiated in > > SchedulerThreadPoolImpl) has it (not the generic interface) and the > > SchedulerWorkerThread thus can't reach it (it's not even aware of its > > existence). > > > > Prior to this there was a map in SchedulerThreadPoolImpl to map > > SchedulerWorkerThreads to their PriorityQueue but I thought it felt silly to > do > > a map lookup to get something we already have hidden in the > > SchedulerWorkerThread we're using to do the query..! > > What if Delegate had a GetThreadPriorityQueue? We would like the SchedulerWorkerThread to be agnostic to priority queues as all manipulations about the priority queues are handled by the delegate. In the implementation, the SchedulerThreadPool fully controls the delegate that gets passed to the SchedulerWorkerThread. It stands to reason that the SchedulerThreadPool should be able to get its own implementation back of the delegate since it's the one that passed it in. I suspect it would be unexpected if the delegate were to change materially after passing it in. The other alternative is to allow two owners of the pointer, either via raw pointer or via refcount. These two I don't like so much.
LGTM https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:71: SchedulerWorkerThread::Delegate* delegate() { return delegate_.get(); } On 2016/04/27 21:14:53, robliao wrote: > On 2016/04/27 20:53:28, danakj wrote: > > On 2016/04/27 20:45:39, gab wrote: > > > On 2016/04/27 20:38:13, danakj wrote: > > > > I also wonder about exposing the delegate here, this seems to be bad for > law > > > of > > > > demeter. Maybe this could return the priority queue, and the fact that it > > goes > > > > through delegate to get it is an implementation detail? > > > > > > I asked for this. Asking the SchedulerWorkerThread for the priority queue > > > doesn't work because the impl (which is defined and instantiated in > > > SchedulerThreadPoolImpl) has it (not the generic interface) and the > > > SchedulerWorkerThread thus can't reach it (it's not even aware of its > > > existence). > > > > > > Prior to this there was a map in SchedulerThreadPoolImpl to map > > > SchedulerWorkerThreads to their PriorityQueue but I thought it felt silly to > > do > > > a map lookup to get something we already have hidden in the > > > SchedulerWorkerThread we're using to do the query..! > > > > What if Delegate had a GetThreadPriorityQueue? > > We would like the SchedulerWorkerThread to be agnostic to priority queues as all > manipulations about the priority queues are handled by the delegate. > > In the implementation, the SchedulerThreadPool fully controls the delegate that > gets passed to the SchedulerWorkerThread. It stands to reason that the > SchedulerThreadPool should be able to get its own implementation back of the > delegate since it's the one that passed it in. I suspect it would be unexpected > if the delegate were to change materially after passing it in. OK that is fair yeah. We do the same thing when we pass things into blink and then down cast them back when blink hands that type back. Though there it's because we're not allowed to expose the real thing to blink. I'm not sure it's ideal, but it's certainly valid. > The other alternative is to allow two owners of the pointer, either via raw > pointer or via refcount. These two I don't like so much. Yeh, I agree.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1876363004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1876363004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:160: for (const auto& current_worker_thread : worker_threads) { On 2016/04/27 20:38:13, danakj wrote: > auto it = std::find_if( > worker_threads.begin(), > worker_threads.end(), > [worker_thread](const std::unique_ptr<SchedulerWorkerThread>& i) { > return i.get() == worker_thread; > }); > return it != worker_threads.end(); > > also works.. but maybe this is cleaner as is, up to you. thought I'd mention it Done. https://codereview.chromium.org/1876363004/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:331: #endif // DCHECK_IS_ON() On 2016/04/27 20:38:12, danakj wrote: > nit: i dont think you need these ending comments if the if block is very short > (~6 lines?) that's what the tooling enforces for namespaces also. Done. https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:422: (!shared_sequence_and_sort_key.is_null() && On 2016/04/27 20:38:13, danakj wrote: > nit: I don't like combining && and || in one if statement, especially when it's > so long. If you can break this into a few bools and use those in the if > statement instead that'd be cool. Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, danakj@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/1876363004/#ps360001 (title: "self review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1876363004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1876363004/360001
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-...)
Nice tweaks in last patch sets :-), cleaner, thanks Dana! Nit below (which I think is the compile error): https://codereview.chromium.org/1876363004/diff/360001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/360001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:160: auto it = std::find_if( #include <algorithm> (I think that's the bot error too)
https://codereview.chromium.org/1876363004/diff/360001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool_impl.cc (right): https://codereview.chromium.org/1876363004/diff/360001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool_impl.cc:160: auto it = std::find_if( On 2016/04/28 13:40:03, gab wrote: > #include <algorithm> > > (I think that's the bot error too) Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, danakj@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/1876363004/#ps380001 (title: "add #include <algorithm>")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1876363004/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1876363004/380001
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [11] Support ExecutionMode::SINGLE_THREADED. Support ExecutionMode::SINGLE_THREADED in SchedulerThreadPool::CreateTaskRunnerWithTraits. All tasks posted to a SINGLE_THREADED TaskRunner run on the same thread in posting order. BUG=553459 ========== to ========== TaskScheduler [11] Support ExecutionMode::SINGLE_THREADED. Support ExecutionMode::SINGLE_THREADED in SchedulerThreadPool::CreateTaskRunnerWithTraits. All tasks posted to a SINGLE_THREADED TaskRunner run on the same thread in posting order. BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/1e7a257a501d2d97fe6550fa023ca2998563c495 Cr-Commit-Position: refs/heads/master@{#390394}
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [11] Support ExecutionMode::SINGLE_THREADED. Support ExecutionMode::SINGLE_THREADED in SchedulerThreadPool::CreateTaskRunnerWithTraits. All tasks posted to a SINGLE_THREADED TaskRunner run on the same thread in posting order. BUG=553459 ========== to ========== TaskScheduler [11] Support ExecutionMode::SINGLE_THREADED. Support ExecutionMode::SINGLE_THREADED in SchedulerThreadPool::CreateTaskRunnerWithTraits. All tasks posted to a SINGLE_THREADED TaskRunner run on the same thread in posting order. BUG=553459 Committed: https://crrev.com/1e7a257a501d2d97fe6550fa023ca2998563c495 Cr-Commit-Position: refs/heads/master@{#390394} ========== |
