Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(356)

Issue 1685423002: Task Scheduler. (Closed)

Created:
4 years, 10 months ago by fdoray
Modified:
4 years, 10 months ago
Reviewers:
robliao
CC:
chromium-reviews, vmpstr+watch_chromium.org
Base URL:
https://luckyluke-private.googlesource.com/src@a_master
Target Ref:
refs/pending/heads/a_master
Project:
chromium
Visibility:
Public.

Description

Task Scheduler.

Patch Set 1 #

Total comments: 80
Unified diffs Side-by-side diffs Delta from patch set Stats (+4387 lines, -0 lines) Patch
M base/base.gyp View 1 chunk +12 lines, -0 lines 2 comments Download
M base/base.gypi View 1 chunk +28 lines, -0 lines 0 comments Download
A base/task_scheduler/delayed_task_manager.h View 1 chunk +92 lines, -0 lines 7 comments Download
A base/task_scheduler/delayed_task_manager.cc View 1 chunk +104 lines, -0 lines 6 comments Download
A base/task_scheduler/delayed_task_manager_unittest.cc View 1 chunk +159 lines, -0 lines 4 comments Download
A base/task_scheduler/post_task.h View 1 chunk +102 lines, -0 lines 5 comments Download
A base/task_scheduler/post_task.cc View 1 chunk +45 lines, -0 lines 2 comments Download
A base/task_scheduler/priority_queue.h View 1 chunk +108 lines, -0 lines 10 comments Download
A base/task_scheduler/priority_queue.cc View 1 chunk +74 lines, -0 lines 2 comments Download
A base/task_scheduler/priority_queue_unittest.cc View 1 chunk +121 lines, -0 lines 0 comments Download
A base/task_scheduler/scheduler_lock.h View 1 chunk +56 lines, -0 lines 0 comments Download
A base/task_scheduler/scheduler_lock.cc View 1 chunk +128 lines, -0 lines 8 comments Download
A base/task_scheduler/scheduler_lock_unittest.cc View 1 chunk +184 lines, -0 lines 0 comments Download
A base/task_scheduler/sequence.h View 1 chunk +64 lines, -0 lines 2 comments Download
A base/task_scheduler/sequence.cc View 1 chunk +65 lines, -0 lines 3 comments Download
A base/task_scheduler/sequence_sort_key.h View 1 chunk +46 lines, -0 lines 0 comments Download
A base/task_scheduler/sequence_sort_key.cc View 1 chunk +34 lines, -0 lines 0 comments Download
A base/task_scheduler/sequence_sort_key_unittest.cc View 1 chunk +72 lines, -0 lines 0 comments Download
A base/task_scheduler/sequence_unittest.cc View 1 chunk +141 lines, -0 lines 0 comments Download
A base/task_scheduler/shutdown_manager.h View 1 chunk +74 lines, -0 lines 0 comments Download
A base/task_scheduler/shutdown_manager.cc View 1 chunk +93 lines, -0 lines 2 comments Download
A base/task_scheduler/shutdown_manager_unittest.cc View 1 chunk +237 lines, -0 lines 0 comments Download
A base/task_scheduler/task.h View 1 chunk +39 lines, -0 lines 0 comments Download
A base/task_scheduler/task.cc View 1 chunk +23 lines, -0 lines 0 comments Download
A base/task_scheduler/task_scheduler.h View 1 chunk +62 lines, -0 lines 0 comments Download
A base/task_scheduler/task_scheduler.cc View 1 chunk +34 lines, -0 lines 0 comments Download
A base/task_scheduler/task_scheduler_impl.h View 1 chunk +63 lines, -0 lines 2 comments Download
A base/task_scheduler/task_scheduler_impl.cc View 1 chunk +99 lines, -0 lines 6 comments Download
A base/task_scheduler/task_scheduler_impl_unittest.cc View 1 chunk +146 lines, -0 lines 0 comments Download
A base/task_scheduler/task_traits.h View 1 chunk +135 lines, -0 lines 2 comments Download
A base/task_scheduler/task_traits.cc View 1 chunk +31 lines, -0 lines 0 comments Download
A base/task_scheduler/test_util.h View 1 chunk +19 lines, -0 lines 0 comments Download
A base/task_scheduler/test_util.cc View 1 chunk +29 lines, -0 lines 0 comments Download
A base/task_scheduler/thread_pool.h View 1 chunk +125 lines, -0 lines 2 comments Download
A base/task_scheduler/thread_pool.cc View 1 chunk +320 lines, -0 lines 4 comments Download
A base/task_scheduler/thread_pool_unittest.cc View 1 chunk +304 lines, -0 lines 0 comments Download
A base/task_scheduler/utils.h View 1 chunk +45 lines, -0 lines 0 comments Download
A base/task_scheduler/utils.cc View 1 chunk +65 lines, -0 lines 0 comments Download
A base/task_scheduler/utils_unittest.cc View 1 chunk +139 lines, -0 lines 0 comments Download
A base/task_scheduler/worker_thread.h View 1 chunk +140 lines, -0 lines 5 comments Download
A base/task_scheduler/worker_thread.cc View 1 chunk +308 lines, -0 lines 6 comments Download
A base/task_scheduler/worker_thread_unittest.cc View 1 chunk +222 lines, -0 lines 0 comments Download

Messages

Total messages: 7 (1 generated)
fdoray
https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed_task_manager.cc File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed_task_manager.cc#newcode29 base/task_scheduler/delayed_task_manager.cc:29: DCHECK_NE(TimeTicks(), task.delayed_run_time); DCHECK(sequence.get()); DCHECK(priority_queue); https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed_task_manager.cc#newcode57 base/task_scheduler/delayed_task_manager.cc:57: // to avoid ...
4 years, 10 months ago (2016-02-11 17:30:34 UTC) #1
robliao
In progress comments so far. https://codereview.chromium.org/1685423002/diff/1/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1685423002/diff/1/base/base.gyp#newcode542 base/base.gyp:542: 'task_scheduler/delayed_task_manager_unittest.cc', We'll need to ...
4 years, 10 months ago (2016-02-11 21:56:27 UTC) #3
robliao
More comments. https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_pool.cc File base/task_scheduler/thread_pool.cc (right): https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_pool.cc#newcode305 base/task_scheduler/thread_pool.cc:305: if (!worker_thread->HasSingleThreadedTasks()) { Maybe a controller pattern ...
4 years, 10 months ago (2016-02-11 22:30:08 UTC) #4
robliao
Next batch of comments. https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_task.cc File base/task_scheduler/post_task.cc (right): https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_task.cc#newcode25 base/task_scheduler/post_task.cc:25: DCHECK(TaskScheduler::GetInstance()); Remove DCHECK and the ...
4 years, 10 months ago (2016-02-11 22:49:30 UTC) #5
robliao
https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/scheduler_lock.cc File base/task_scheduler/scheduler_lock.cc (right): https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/scheduler_lock.cc#newcode49 base/task_scheduler/scheduler_lock.cc:49: const auto& iter = On 2016/02/11 17:30:33, fdoray wrote: ...
4 years, 10 months ago (2016-02-11 22:59:04 UTC) #6
fdoray
4 years, 10 months ago (2016-02-12 04:16:20 UTC) #7
https://codereview.chromium.org/1685423002/diff/1/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1685423002/diff/1/base/base.gyp#newcode542
base/base.gyp:542: 'task_scheduler/delayed_task_manager_unittest.cc',
On 2016/02/11 21:56:27, robliao wrote:
> We'll need to do this for BUILD.gn as well.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
File base/task_scheduler/delayed_task_manager.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.cc:29: DCHECK_NE(TimeTicks(),
task.delayed_run_time);
On 2016/02/11 17:30:32, fdoray wrote:
> DCHECK(sequence.get());
> DCHECK(priority_queue);

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.cc:57: // to avoid holding 2 locks at
the same time.
On 2016/02/11 17:30:32, fdoray wrote:
> multiple locks

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.cc:58: TimeTicks now = Now();
On 2016/02/11 17:30:32, fdoray wrote:
> const TimeTicks now = Now();

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
File base/task_scheduler/delayed_task_manager.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.h:20: namespace task_scheduler {
On 2016/02/11 21:56:27, robliao wrote:
> On 2016/02/11 17:30:32, fdoray wrote:
> > Could we have a subtle namespace for things that people shouldn't use?
> 
> Maybe it's the return of the internal namespace.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.h:48: TimeTicks
GetNextDelayedTaskReadyTime() const;
On 2016/02/11 17:30:32, fdoray wrote:
> GetNextDelayedRunTime()

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager.h:76: // |delayed_run_time| is in front
of the priority queue.
On 2016/02/11 17:30:32, fdoray wrote:
> task.delayed_run_time

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
File base/task_scheduler/delayed_task_manager_unittest.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager_unittest.cc:52: // sequence and
priority queue by PostReadyTasks() when the current time is
On 2016/02/11 17:30:33, fdoray wrote:
> current time *becomes

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/delayed...
base/task_scheduler/delayed_task_manager_unittest.cc:80: // sequence and
priority queue by PostReadyTasks() when the current time is
On 2016/02/11 17:30:32, fdoray wrote:
> current time *becomes

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
File base/task_scheduler/post_task.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
base/task_scheduler/post_task.cc:25: DCHECK(TaskScheduler::GetInstance());
On 2016/02/11 22:49:30, robliao wrote:
> Remove DCHECK and the one in CreateTaskRunnerWithTraits.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
File base/task_scheduler/post_task.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
base/task_scheduler/post_task.h:95: // CreateTaskRunnerWithTraits are supported.
On 2016/02/11 17:30:33, fdoray wrote:
> remove "are supported"

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/post_ta...
base/task_scheduler/post_task.h:98: TaskTraits traits);
On 2016/02/11 22:49:30, robliao wrote:
> On 2016/02/11 17:30:33, fdoray wrote:
> > How will we know that |parent_task_runner| is SEQUENCED?
> 
> That's a good question. We currently don't have a good way to introspect into
> this pointer and enforce that the TaskRunners are actually our task runners.
> We could have either all task runners we create extend from a TaskRunnerType
> class that provides one virtual method to return the TaskRunnerType.
> 
> Alternatively, we could create an unordered_map of all TaskRunners we've
handed
> out. We would have to use WeakPtrs for this and it would probably be pretty
> ugly.

Solution 1 seems good but... ref-counted classes can't use multiple inheritance
and our TaskRunners already inherit from (Sequenced|SingleThread)TaskRunner.
Maybe we could have all our TaskRunners inherit from SchedulerTaskRunner which
would itself inherit from TaskRunner (no more
SequencedTaskRunner/SingleThreadTaskRunner inheritance). We would need special
TaskRunners for (Thread|Sequenced)TaskRunnerHandle.

Solution 2 is indeed ugly.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
File base/task_scheduler/priority_queue.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.cc:45: SequenceSortKey* sort_key) const {
On 2016/02/11 17:30:33, fdoray wrote:
> DCHECK(sort_key);

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
File base/task_scheduler/priority_queue.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:27: // |sequence_inserted_callback| is
invoked whenever a sequence is added to the
On 2016/02/11 17:30:33, fdoray wrote:
> is *pushed* to the priority queue

WontFix

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:38: // calling this to avoid getting a
stale result. The returned result can be
On 2016/02/11 17:30:33, fdoray wrote:
> to *mitigate the chances* of getting a stale result

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:43: class BASE_EXPORT Transaction {
On 2016/02/11 17:30:33, fdoray wrote:
> : public NonThreadSafe

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:79: // modify the priority queue while the
returned Transaction is alive. The
On 2016/02/11 17:30:33, fdoray wrote:
> The *returned Transaction

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/priorit...
base/task_scheduler/priority_queue.h:86: // Controls access to
|priority_queue_|.
On 2016/02/11 17:30:33, fdoray wrote:
> to |container_|

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequenc...
File base/task_scheduler/sequence.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequenc...
base/task_scheduler/sequence.cc:18: DCHECK(task.get());
On 2016/02/11 22:49:30, robliao wrote:
> On 2016/02/11 17:30:33, fdoray wrote:
> > DCHECK first?
> > + DCHECK(prev_num_tasks);
> 
> Both DCHECKs are unnecessary as both will crash below.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequence.h
File base/task_scheduler/sequence.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/sequenc...
base/task_scheduler/sequence.h:18: #include "base/task_scheduler/task.h"
On 2016/02/11 17:30:33, fdoray wrote:
> #include "task_traits.h" for kNumTaskPriorities.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/shutdow...
File base/task_scheduler/shutdown_manager.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/shutdow...
base/task_scheduler/shutdown_manager.cc:58: ++num_tasks_blocking_shutdown_;
On 2016/02/11 22:49:30, robliao wrote:
> Should this be here?

I think the code is correct. A SKIP_ON_SHUTDOWN task scheduled before shutdown
has started blocks shutdown.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
File base/task_scheduler/task_scheduler_impl.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
base/task_scheduler/task_scheduler_impl.cc:17:
Bind(&TaskSchedulerImpl::ReinsertSequenceCallback, Unretained(this));
On 2016/02/11 21:56:27, robliao wrote:
> We'll want to comment that the TaskScheduler lives forever, so Unretained is
> safe here.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
base/task_scheduler/task_scheduler_impl.cc:46: // TODO(fdoray): Support
WithSequenceToken().
On 2016/02/11 21:56:27, robliao wrote:
> Remove this comment.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
base/task_scheduler/task_scheduler_impl.cc:92: traits.WithFileIO();
On 2016/02/11 21:56:27, robliao wrote:
> traits = traits.WithFileIO() to avoid the return value ignore.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
File base/task_scheduler/task_scheduler_impl.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_sc...
base/task_scheduler/task_scheduler_impl.h:38: void JoinAllThreadsForTesting();
On 2016/02/11 21:56:27, robliao wrote:
> Is it ever valid to call this without calling Shutdown()?
> 
> Maybe this can be ShutdownAndJoinAllThreadsForTesting().

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_tr...
File base/task_scheduler/task_traits.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_traits.h:110: enum class ExecutionMode {
On 2016/02/11 17:30:33, fdoray wrote:
> Add a comment explaining that this is used to create task runners?

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
File base/task_scheduler/thread_pool.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
base/task_scheduler/thread_pool.cc:262: void
ThreadPool::WorkerThreadBecomesIdleCallback(WorkerThread* worker_thread) {
On 2016/02/11 17:30:33, fdoray wrote:
> DCHECK that |worker_thread| belongs to this thread pool?

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
base/task_scheduler/thread_pool.cc:305: if
(!worker_thread->HasSingleThreadedTasks()) {
On 2016/02/11 22:30:07, robliao wrote:
> Maybe a controller pattern might make reasoning over the global state of the
> threadpool easier.

I agree. I'll try to write this (tomorrow in the plane?) and we'll see if it
makes the code simpler. Since threads already have to lock
|shared_priority_queue_| each time that they get work, I don't think that having
an extra lock to protect a manager which knows about the state of each thread in
a pool will harm parallelism.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
File base/task_scheduler/thread_pool.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/thread_...
base/task_scheduler/thread_pool.h:72: // the threads won't exit. This method is
not thread-safe.
On 2016/02/11 17:30:33, fdoray wrote:
> "This method can only be called once per ThreadPool."

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
File base/task_scheduler/worker_thread.cc (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.cc:247: for (;;) {
On 2016/02/11 21:56:27, robliao wrote:
> Maybe this should be while(!shutdown_manager->shutdown_completed())

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.cc:260: sequence = GetWork();
On 2016/02/11 22:30:07, robliao wrote:
> Can we remove this somehow?

We could if we built something that atomically checks if there is work to do and
calls WaitUntilWorkIsAvailable if there isn't. I'll work on this tomorrow.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.cc:271: DCHECK(task);
On 2016/02/11 22:30:07, robliao wrote:
> Remove DCHECK.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
File base/task_scheduler/worker_thread.h (right):

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.h:72: bool HasSingleThreadedTasks() const;
On 2016/02/11 21:56:27, robliao wrote:
> Since we're claiming this is thread safe, maybe we should just put the memory
> barrier in the implementation.

Done.

https://codereview.chromium.org/1685423002/diff/1/base/task_scheduler/worker_...
base/task_scheduler/worker_thread.h:76: // method is not thread-safe.
On 2016/02/11 21:56:27, robliao wrote:
> On 2016/02/11 17:30:34, fdoray wrote:
> > This can only be called once per WorkerThread.
> 
> We should be able to assert this.

Done. I added a DCHECK, which is unfortunately not thread-safe. Do you think
that I should add a lock just to make sure that a method which is supposed to be
used in tests only isn't called multiple times?

Powered by Google App Engine
This is Rietveld 408576698