TaskScheduler [7] SchedulerThreadPool

base/task_scheduler/scheduler_thread_pool.h

Index: base/task_scheduler/scheduler_thread_pool.h
diff --git a/base/task_scheduler/scheduler_thread_pool.h b/base/task_scheduler/scheduler_thread_pool.h
new file mode 100644
index 0000000000000000000000000000000000000000..1385731f82ae317dff1af32fb87afbacac030747
--- /dev/null
+++ b/base/task_scheduler/scheduler_thread_pool.h
@@ -0,0 +1,128 @@
+// 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_THREAD_POOL_H_
+#define BASE_TASK_SCHEDULER_SCHEDULER_THREAD_POOL_H_
+
+#include <stddef.h>
+
+#include <memory>
+#include <stack>
+#include <vector>
+
+#include "base/base_export.h"
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/synchronization/condition_variable.h"
+#include "base/task_runner.h"
+#include "base/task_scheduler/priority_queue.h"
+#include "base/task_scheduler/scheduler_lock.h"
+#include "base/task_scheduler/scheduler_worker_thread.h"
+#include "base/task_scheduler/sequence.h"
+#include "base/task_scheduler/task_traits.h"
+#include "base/threading/platform_thread.h"
+
+namespace base {
+namespace internal {
+
+struct SequenceSortKey;
+class TaskTracker;
+
+// A pool of threads that run Tasks. This class is thread-safe.
+class BASE_EXPORT SchedulerThreadPool : public SchedulerWorkerThread::Delegate {

[danakj, 2016/04/07 23:08:44]

nit: maybe consider putting the delegate in its own header file and not nesting
it (avoids this header needing to know about the whole SchedulerWorkerThread).
I've been frustrated over time with public nested types in general, so just a
feeling it might help you one day. feel free to leave it too.

[fdoray, 2016/04/08 14:53:03]

gab@: You asked me to nest Delegate inside SchedulerWorkerThread. Do you mind if
we move it back to its own .h file?

[gab, 2016/04/08 17:56:00]

I think it's fine as an inner class, @dana: was that your concern or more about
it being merged direclty with the public API?

And as we discussed you might even be able to put it all in the .cc file?

[danakj, 2016/04/08 18:05:32]

Not that, but that nested classes can't be forward declared, so they have weird
implications for other files that want to use them. It's been annoying in the
past in cc/

[fdoray, 2016/04/08 19:00:05]

I'll move SchedulerWorkerThread::Delegate to its own header file in a separate
CL.

[gab, 2016/04/08 21:10:50]

Hmmm but why would another file want to use the Delegate? It's an implementation
detail of this class (i.e. I think we can put it all in the .cc file except for
a mere fwd-decl in this header to declare it as an inner class).

[fdoray, 2016/04/11 13:52:23]

I will move the definition of *SchedulerWorkerThread::Delegate* (base class) to
its own file. The definition of SchedulerWorkerThreadDelegateImpl will stay in
scheduler_thread_pool.cc.

+ public:
+  // Callback invoked after a worker thread has tried to run a Task from
+  // |sequence| (a TaskTracker might have prevented the Task from running).
+  using RanTaskFromSequenceCallback = Callback<void(scoped_refptr<Sequence>)>;
+
+  // Creates a SchedulerThreadPool with up to |max_threads| threads of priority
+  // |thread_priority|. |ran_task_from_sequence_callback| will be invoked after
+  // a thread of this thread pool tries to run a Task. |task_tracker| is used to
+  // handle shutdown behavior of Tasks. Returns nullptr when it's not possible
+  // to create a thread pool with at least one thread.
+  static std::unique_ptr<SchedulerThreadPool> CreateThreadPool(
+      ThreadPriority thread_priority,
+      size_t max_threads,
+      const RanTaskFromSequenceCallback& ran_task_from_sequence_callback,
+      TaskTracker* task_tracker);
+
+  // Destroying a SchedulerThreadPool returned by CreateThreadPool() is not
+  // allowed in production; it is always leaked. In tests, it can only be
+  // destroyed after JoinForTesting() has returned.
+  ~SchedulerThreadPool() override;
+
+  // Returns a TaskRunner whose PostTask invocations will result in scheduling
+  // Tasks with |traits| and |execution_mode| in this thread pool.
+  scoped_refptr<TaskRunner> CreateTaskRunnerWithTraits(
+      const TaskTraits& traits,
+      ExecutionMode execution_mode);
+
+  // Inserts |sequence| into this thread pool's shared priority queue with
+  // |sequence_sort_key|. Must only be used by a worker thread to put |sequence|
+  // back into a PriorityQueue after running a Task from it.
+  void InsertSequenceAfterTaskRan(scoped_refptr<Sequence> sequence,
+                                  const SequenceSortKey& sequence_sort_key);
+
+  // Waits until all threads are idle.
+  void WaitForAllWorkerThreadsIdleForTesting();
+
+  // Joins all threads of this thread pool. Tasks that are already running are
+  // allowed to complete their execution. This can only be called once.
+  void JoinForTesting();
+
+ private:
+  SchedulerThreadPool(
+      const RanTaskFromSequenceCallback& ran_task_from_sequence_callback,
+      TaskTracker* task_tracker);
+
+  void Initialize(ThreadPriority thread_priority, size_t max_threads);
+
+  // Wakes up one thread from this thread pool if they aren't all busy.
+  void WakeUpOneThread();
+
+  // Adds |worker_thread| to |idle_worker_threads_stack_|.
+  void AddToIdleWorkerThreadsStack(SchedulerWorkerThread* worker_thread);
+
+  // SchedulerWorkerThread::Delegate:

[danakj, 2016/04/07 23:08:44]

nit: not really sure why these are private, you can upcast and call them
anyways. and they are called from outside this class as the delegate, so it's a
bit misleading maybe.

[fdoray, 2016/04/08 14:53:03]

I'd like to keep these methods private to document that they shouldn't shouldn't
be called from a SchedulerThreadPool*. If you upcast to
SchedulerWorkerThread::Delegate* outside of SchedulerWorkerThread, it's a hint
that you are doing something wrong. Relevant discussion:
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/CwzjcWd9cYM...

If you disagree with this, I'll make the methods public.

[fdoray, 2016/04/08 15:41:10]

Moved all these methods to SchedulerWorkerThreadDelegateImpl. This solves the
visibility problem that you pointed out and avoids having two EnqueueSequence
methods in SchedulerThreadPool (one that forwards to enqueue_sequence_callback_
and one that actually enqueues the sequences it receives).

+  void OnMainEntry() override;
+  void OnMainExit() override;
+  scoped_refptr<Sequence> GetWork(
+      SchedulerWorkerThread* worker_thread) override;
+  void RanTaskFromSequence(scoped_refptr<Sequence> sequence) override;
+
+  // PriorityQueue from which all threads of this thread pool get work.
+  PriorityQueue shared_priority_queue_;
+
+  // All worker threads owned by this thread pool. This is only modified by the
+  // constructor.
+  std::vector<std::unique_ptr<SchedulerWorkerThread>> worker_threads_;
+
+  // Synchronizes access to |idle_worker_threads_stack_| and
+  // |idle_worker_threads_cv_|.
+  SchedulerLock idle_worker_threads_stack_lock_;
+
+  // Stack of idle worker threads. The last worker thread that became idle is on
+  // top of the stack.
+  std::stack<SchedulerWorkerThread*> idle_worker_threads_stack_;
+
+  // Signaled when all worker threads become idle.
+  std::unique_ptr<ConditionVariable> idle_worker_threads_stack_cv_;
+
+  // Synchronizes access to |join_for_testing_returned_|.
+  SchedulerLock join_for_testing_returned_lock_;
+
+  // True once JoinForTesting() has returned.
+  bool join_for_testing_returned_ = false;
+
+  const RanTaskFromSequenceCallback ran_task_from_sequence_callback_;
+  TaskTracker* const task_tracker_;
+
+  DISALLOW_COPY_AND_ASSIGN(SchedulerThreadPool);
+};
+
+}  // namespace internal
+}  // namespace base
+
+#endif  // BASE_TASK_SCHEDULER_SCHEDULER_THREAD_POOL_H_