Add Lazy Creation and Thread Detachment Support in the Scheduler Worker Pool

base/task_scheduler/scheduler_worker_pool_impl.h

Index: base/task_scheduler/scheduler_worker_pool_impl.h
diff --git a/base/task_scheduler/scheduler_worker_pool_impl.h b/base/task_scheduler/scheduler_worker_pool_impl.h
index 935c79a80c5f000b549628399e0a51dc1180f1ce..e9abf597830cde7e3fa829fcbc516ffe39b5be2b 100644
--- a/base/task_scheduler/scheduler_worker_pool_impl.h
+++ b/base/task_scheduler/scheduler_worker_pool_impl.h
@@ -30,6 +30,9 @@
 #include "base/threading/platform_thread.h"
 
 namespace base {
+
+class TimeDelta;
+
 namespace internal {
 
 class DelayedTaskManager;
@@ -55,16 +58,19 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
   // Creates a SchedulerWorkerPoolImpl labeled |name| with up to |max_threads|
   // threads of priority |thread_priority|. |io_restriction| indicates whether
   // Tasks on the constructed worker pool are allowed to make I/O calls.
-  // |re_enqueue_sequence_callback| will be invoked after a worker of this
-  // worker pool tries to run a Task. |task_tracker| is used to handle shutdown
-  // behavior of Tasks. |delayed_task_manager| handles Tasks posted with a
-  // delay. Returns nullptr on failure to create a worker pool with at least one
-  // thread.
+  // |suggested_reclaim_time| sets a suggestion on when to reclaim idle threads.
+  // The worker pool is free to ignore this value for performance or correctness
+  // reasons. |re_enqueue_sequence_callback| will be invoked after a worker of
+  // this worker pool tries to run a Task. |task_tracker| is used to handle
+  // shutdown behavior of Tasks. |delayed_task_manager| handles Tasks posted
+  // with a delay. Returns nullptr on failure to create a worker pool with at
+  // least one thread.
   static std::unique_ptr<SchedulerWorkerPoolImpl> Create(
       StringPiece name,
       ThreadPriority thread_priority,
       size_t max_threads,
       IORestriction io_restriction,
+      const TimeDelta& suggested_reclaim_time,
       const ReEnqueueSequenceCallback& re_enqueue_sequence_callback,
       TaskTracker* task_tracker,
       DelayedTaskManager* delayed_task_manager);
@@ -76,6 +82,11 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
   // allowed to complete their execution. This can only be called once.
   void JoinForTesting();
 
+  // Disallows worker thread detachment. It's recommended that this should be
+  // called before dispatching the first set of work to avoid detach and join
+  // races.
+  void DisallowWorkerDetachmentForTesting();

[gab, 2016/07/13 18:36:31]

It seems weird to me that something that is "recommended" not be highlighted
more than this. I see though that JoinForTesting() ensures this is called (but
it can't fully confirm it was called early enough). So perhaps tweak the comment
to something like:

// Disallows worker thread detachment. In tests, this must be called before the
last synchronization point with all worker threads (e.g. before having them sync
on the last WaitableEvent or even before dispatching the first set of work)
before calling JoinForTesting() or detach/join races may ensue.

Actually, this makes me think, should we not instead get rid of Detach/Join
races? I guess that adds a  bit of test specific synchronization logic to
Detach() in non-test code but since the thread is about to Detach(), it doesn't
need to be efficient and the Join() side is only for testing so wtv.

I think I prefer that to the complexity involved in this required
DisallowWorkerDetachmentForTesting() call.

[robliao, 2016/07/13 20:19:47]

I've amended the comment. Tests that specify a reclaim time of TimeDelta::Max()
don't need to call this function. 

The reason why I think we should avoid fixing the race is it only applies to
tests that allow detachment (and as of the new patchset, it's only the tests
that test for detachment). Given that the test is looking for detachment, it
already has to construct the worker pool where it can inspect state at certain
timed points. 

All other tests can specify TimeDelta::Max() to avoid the issue entirely.

+
   // SchedulerWorkerPool:
   scoped_refptr<TaskRunner> CreateTaskRunnerWithTraits(
       const TaskTraits& traits,
@@ -90,10 +101,12 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
                                SchedulerWorker* worker) override;
 
  private:
+  class SchedulerSingleThreadTaskRunner;
   class SchedulerWorkerDelegateImpl;
 
   SchedulerWorkerPoolImpl(StringPiece name,
                           IORestriction io_restriction,
+                          const TimeDelta& suggested_reclaim_time,
                           TaskTracker* task_tracker,
                           DelayedTaskManager* delayed_task_manager);
 
@@ -108,9 +121,15 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
   // Adds |worker| to |idle_workers_stack_|.
   void AddToIdleWorkersStack(SchedulerWorker* worker);
 
+  // Peeks from |idle_workers_stack_|.
+  const SchedulerWorker* PeekAtIdleWorkersStack() const;
+
   // Removes |worker| from |idle_workers_stack_|.
   void RemoveFromIdleWorkersStack(SchedulerWorker* worker);
 
+  // Returns true if worker thread detachment is permitted.
+  bool CanWorkerDetachForTesting();
+
   // The name of this worker pool, used to label its worker threads.
   const std::string name_;
 
@@ -131,12 +150,15 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
   // Indicates whether Tasks on this worker pool are allowed to make I/O calls.
   const IORestriction io_restriction_;
 
+  // Suggested reclaim time for workers.
+  const TimeDelta suggested_reclaim_time_;
+
   // Synchronizes access to |idle_workers_stack_| and
   // |idle_workers_stack_cv_for_testing_|. Has |shared_priority_queue_|'s
   // lock as its predecessor so that a worker can be pushed to
   // |idle_workers_stack_| within the scope of a Transaction (more
   // details in GetWork()).
-  SchedulerLock idle_workers_stack_lock_;
+  mutable SchedulerLock idle_workers_stack_lock_;
 
   // Stack of idle workers.
   SchedulerWorkerStack idle_workers_stack_;
@@ -147,6 +169,13 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
   // Signaled once JoinForTesting() has returned.
   WaitableEvent join_for_testing_returned_;
 
+  // Synchronizes access to |worker_detachment_allowed_|.
+  SchedulerLock worker_detachment_allowed_lock_;
+
+  // Indicates to the delegates that workers are permitted to detach their
+  // threads.
+  bool worker_detachment_allowed_;

[gab, 2016/07/13 18:36:31]

Use a manual WaitableEvent instead of bool + lock? (this is what we did in many
other similar scenarios in scheduler)

[robliao, 2016/07/13 20:19:47]

Discussed here:
https://codereview.chromium.org/2116163002/diff/1/base/task_scheduler/schedul...

In short, it's an abuse of WaitableEvent since we're never actually going to
wait on this value. What we really want is something like AtomicBool. Until
then, this is the next best thing.

+
 #if DCHECK_IS_ON()
   // Signaled when all workers have been created.
   WaitableEvent workers_created_;