Virtualize The Existence of a Scheduler Worker Thread

base/task_scheduler/scheduler_worker_thread.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_SCHEDULER_WORKER_THREAD_H_
 #define BASE_TASK_SCHEDULER_SCHEDULER_WORKER_THREAD_H_
 
 #include <memory>
 
 #include "base/base_export.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/task_scheduler/scheduler_lock.h"
 #include "base/task_scheduler/sequence.h"
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 
 namespace base {
+
 namespace internal {
 
 class TaskTracker;
 
 // A thread that runs Tasks from Sequences returned by a delegate.

[fdoray, 2016/06/08 17:51:48]

s/SchedulerWorkerThread/SchedulerWorker/ ?
This is no longer a thread. It's a "worker" that runs tasks asynchronously, one
at a time, not necessarily on the same thread.

[robliao, 2016/06/08 19:00:08]

This comes down to if the thread should be virtual to the thread pool or not. I
took the stance that as far as the threadpool is concerned, these are threads.
How they are managed underneath the hood is up to the implementation.

I'll give you that it may not necessarily correspond to the same PlatformThread,
and I'll call that out here.

 //
 // A SchedulerWorkerThread starts out sleeping. It is woken up by a call to
 // WakeUp(). After a wake-up, a SchedulerWorkerThread runs Tasks from Sequences
 // returned by the GetWork() method of its delegate as long as it doesn't return
 // nullptr. It also periodically checks with its TaskTracker whether shutdown
 // has completed and exits when it has.
 //
 // This class is thread-safe.
-class BASE_EXPORT SchedulerWorkerThread : public PlatformThread::Delegate {
+class BASE_EXPORT SchedulerWorkerThread {
  public:
   // Delegate interface for SchedulerWorkerThread. The methods are always called
   // from the thread managed by the SchedulerWorkerThread instance.
   class Delegate {
    public:
     virtual ~Delegate() = default;
 
     // Called by |worker_thread| when it enters its main function.

[fdoray, 2016/06/08 17:51:48]

Comment that this is called every time a "real" thread is created. A call to
WakeUp() after the thread was detached (or hibernated) triggers a call to this
method.

[robliao, 2016/06/08 19:00:09]

Updated comment.

     virtual void OnMainEntry(SchedulerWorkerThread* worker_thread) = 0;
 
     // Called by |worker_thread| to get a Sequence from which to run a Task.
     virtual scoped_refptr<Sequence> GetWork(
         SchedulerWorkerThread* worker_thread) = 0;
 
     // Called when |sequence| isn't empty after the SchedulerWorkerThread pops a
     // Task from it. |sequence| is the last Sequence returned by GetWork().
     virtual void ReEnqueueSequence(scoped_refptr<Sequence> sequence) = 0;
 
     // Called by |worker_thread| to determine how long to sleep before the next
     // call to GetWork(). GetWork() may be called before this timeout expires
     // if the thread's WakeUp() method is called.
     virtual TimeDelta GetSleepTimeout() = 0;
+
+    // Called by |worker_thread| to see if it is allowed to detach.

[fdoray, 2016/06/08 17:51:48]

s/CanDetach/CanHibernate/ ? I think those who implement this method should ask
themselves "can this worker enter a state in which it consumes less resources
but takes longer to respond to a WakeUp()?" rather "can the thread handle be
released?" (implementation detail).

Add comments about the side-effects of returning true:
- Future Tasks may run on a different thread than Tasks that already ran.
- The next WakeUp() will be more costly.
- The worker will free resources.

[robliao, 2016/06/08 19:00:08]

Hibernation in the natural sense doesn't actually release resources. It just
shuffles them around or consumes them more slowly.

We're actually releasing resources when we let go of the thread. 

Added the bullet points about detachment.

+    virtual bool CanDetach(SchedulerWorkerThread* worker_thread) = 0;

[fdoray, 2016/06/08 17:51:48]

This is called after GetWork() returns nullptr.

[robliao, 2016/06/08 19:00:09]

Updated loosing the ordering constraint.

   };
 
+  enum class InitialWorkerState { ALIVE, DETACHED };

[fdoray, 2016/06/08 17:51:49]

ALIVE, HIBERNATING

[robliao, 2016/06/08 19:00:09]

See discussion about hibernation vs detachment.

+
   // Creates a SchedulerWorkerThread with priority |thread_priority| that runs
   // Tasks from Sequences returned by |delegate|. |task_tracker| is used to
   // handle shutdown behavior of Tasks. Returns nullptr if creating the
   // underlying platform thread fails.
   static std::unique_ptr<SchedulerWorkerThread> Create(
       ThreadPriority thread_priority,
       std::unique_ptr<Delegate> delegate,
-      TaskTracker* task_tracker);
+      TaskTracker* task_tracker,
+      InitialWorkerState worker_state);

[fdoray, 2016/06/08 17:51:49]

I would remove this argument and always start HIBERNATING.

Advantages:
- WakeUp() is the only method from which Worker is instantiated.
- Simpler and less confusing interface (If WakeUp() needs to be called before
the SchedulerWorkerThread runs tasks, what are the benefits of creating it
ALIVE?)

[robliao, 2016/06/08 19:00:08]

The main benefit here is that it allows the SchedulerThreadPool to ensure that
at least one thread is good to go during create. At the moment, a
SchedulerThreadPool does not have the ability to determine if a real thread is
backing the SchedulerWorkerThread or not.

Updated comment.

 
   // Destroying a SchedulerWorkerThread in production is not allowed; it is
   // always leaked. In tests, it can only be destroyed after JoinForTesting()
   // has returned.
-  ~SchedulerWorkerThread() override;
+  ~SchedulerWorkerThread();
 
   // Wakes up this SchedulerWorkerThread if it wasn't already awake. After this
   // is called, this SchedulerWorkerThread will run Tasks from Sequences
   // returned by the GetWork() method of its delegate until it returns nullptr.

[fdoray, 2016/06/08 17:51:49]

May fail if the thread was hibernating.

[robliao, 2016/06/08 19:00:09]

Updated comment.

   void WakeUp();
 
   SchedulerWorkerThread::Delegate* delegate() { return delegate_.get(); }
 
   // Joins this SchedulerWorkerThread. If a Task is already running, it will be
   // allowed to complete its execution. This can only be called once.
   void JoinForTesting();
 
+  // Whether is worker is alive.

[fdoray, 2016/06/08 17:51:48]

// Returns true if the worker is alive.
+ const

[robliao, 2016/06/08 19:00:09]

I think I was falling into java style habits with these names. Changed to return
true.

+  bool WorkerAliveForTesting();
+
  private:
+  class Worker;
+
   SchedulerWorkerThread(ThreadPriority thread_priority,
                         std::unique_ptr<Delegate> delegate,
                         TaskTracker* task_tracker);
 
-  // PlatformThread::Delegate:
-  void ThreadMain() override;
+  // Returns the worker for ownership by the worker thread if the detach was
+  // successful. If the detach is not possible, returns nullptr.
+  std::unique_ptr<SchedulerWorkerThread::Worker> Detach();
+
+  void CreateWorker();
+
+  void CreateWorkerAssertSynchronized();
 
   bool ShouldExitForTesting() const;
 
-  // Platform thread managed by this SchedulerWorkerThread.
-  PlatformThreadHandle thread_handle_;
+  // Synchronizes access to |worker_|
+  SchedulerLock worker_lock_;
 
-  // Event signaled to wake up this SchedulerWorkerThread.
-  WaitableEvent wake_up_event_;
+  // The underlying thread for this SchedulerWorkerThread.
+  std::unique_ptr<Worker> worker_;
 
+  const ThreadPriority thread_priority_;
   const std::unique_ptr<Delegate> delegate_;
   TaskTracker* const task_tracker_;
 
   // Synchronizes access to |should_exit_for_testing_|.
   mutable SchedulerLock should_exit_for_testing_lock_;
 
   // True once JoinForTesting() has been called.
   bool should_exit_for_testing_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(SchedulerWorkerThread);
 };
 
 }  // namespace internal
 }  // namespace base
 
 #endif  // BASE_TASK_SCHEDULER_SCHEDULER_WORKER_THREAD_H_