TaskScheduler [11] Support ExecutionMode::SINGLE_THREADED.

base/task_scheduler/delayed_task_manager.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_TASK_SCHEDULER_DELAYED_TASK_MANAGER_H_
 #define BASE_TASK_SCHEDULER_DELAYED_TASK_MANAGER_H_
 
 #include <stdint.h>
 
 #include <memory>
 #include <queue>
 #include <vector>
 
 #include "base/base_export.h"
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/task_scheduler/scheduler_lock.h"
 #include "base/task_scheduler/sequence.h"
 #include "base/task_scheduler/task.h"
 #include "base/time/time.h"
 
 namespace base {
 namespace internal {
 
 class SchedulerThreadPool;
+class SchedulerWorkerThread;
 
 // A DelayedTaskManager holds delayed Tasks until they become ripe for
-// execution. When they become ripe for execution, it posts them to their
-// associated Sequence and SchedulerThreadPool. This class is thread-safe.
+// execution. This class is thread-safe.
 class BASE_EXPORT DelayedTaskManager {
  public:
   // |on_delayed_run_time_updated| is invoked when the delayed run time is
   // updated as a result of adding a delayed task to the manager.
   explicit DelayedTaskManager(const Closure& on_delayed_run_time_updated);
   ~DelayedTaskManager();
 
   // Adds |task| to a queue of delayed tasks. The task will be posted to
-  // |thread_pool| as part of |sequence| the first time that PostReadyTasks() is
-  // called while Now() is passed |task->delayed_run_time|.
+  // |thread_pool| with |sequence| and |worker_thread| the first time that
+  // PostReadyTasks() is called while Now() is passed |task->delayed_run_time|.
+  // |worker_thread| is optional.

[gab, 2016/04/25 21:00:20]

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"?

[fdoray, 2016/04/25 22:34:44]

Done.

   //
-  // TODO(robliao): Find a concrete way to manage |thread_pool|'s memory. It is
-  // never deleted in production, but it is better not to spread this assumption
-  // throughout the scheduler.
+  // TODO(robliao): Find a concrete way to manage the memory of |worker_thread|
+  // and |thread_pool|. These objects are never deleted in production, but it is
+  // better not to spread this assumption throughout the scheduler.
   void AddDelayedTask(std::unique_ptr<Task> task,
                       scoped_refptr<Sequence> sequence,
+                      SchedulerWorkerThread* worker_thread,

[gab, 2016/04/25 21:00:20]

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...

[robliao, 2016/04/25 21:42:38]

Alternatively we could have two delayed tasks methods.
AddDelayedTask and AddSingleThreadedDelayedTask. I'm actually leaning towards
this since it avoids the nullptr fun.

[fdoray, 2016/04/25 22:34:44]

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.

[gab, 2016/04/26 11:46:27]

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).

[robliao, 2016/04/26 16:58:56]

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) {}

[gab, 2016/04/26 20:30:58]

Good point, I'd be okay with that then. Maybe in a follow-up CL to avoid churn
all over this one again?

[fdoray, 2016/04/27 16:09:59]

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.

[robliao, 2016/04/27 17:43:45]

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.

[gab, 2016/04/27 18:11:43]

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

[robliao, 2016/04/27 18:21:03]

Yup, agreed on the default argument points. I'm fine with the nullptr as is for
now. Just replying to the comment.

[fdoray, 2016/04/27 19:17:04]

Keeping this discussion for another CL.

                       SchedulerThreadPool* thread_pool);
 
   // Posts delayed tasks that are ripe for execution.
   // TODO(robliao): Call this from a service thread.
   void PostReadyTasks();
 
   // Returns the next time at which a delayed task will become ripe for
   // execution, or a null TimeTicks if there are no pending delayed tasks.
   TimeTicks GetDelayedRunTime() const;
 
   // Returns the current time. Can be overridden for tests.
   virtual TimeTicks Now() const;
 
  private:
   struct DelayedTask;

[robliao, 2016/04/25 21:42:38]

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.

[fdoray, 2016/04/25 22:34:44]

see previous comment

   struct DelayedTaskComparator {
     bool operator()(const DelayedTask& left, const DelayedTask& right) const;
   };
 
   const Closure on_delayed_run_time_updated_;
 
   // Synchronizes access to all members below.
   mutable SchedulerLock lock_;
 
   // Priority queue of delayed tasks. The delayed task with the smallest
   // |task->delayed_run_time| is in front of the priority queue.
   using DelayedTaskQueue = std::priority_queue<DelayedTask,
                                                std::vector<DelayedTask>,
                                                DelayedTaskComparator>;
   DelayedTaskQueue delayed_tasks_;
 
   // The index to assign to the next delayed task added to the manager.
   uint64_t delayed_task_index_ = 0;
 
   DISALLOW_COPY_AND_ASSIGN(DelayedTaskManager);
 };
 
 }  // namespace internal
 }  // namespace base
 
 #endif  // BASE_TASK_SCHEDULER_DELAYED_TASK_MANAGER_H_