TaskScheduler [6] SchedulerWorkerThread

base/task_scheduler/worker_thread.h

Index: base/task_scheduler/worker_thread.h
diff --git a/base/task_scheduler/worker_thread.h b/base/task_scheduler/worker_thread.h
new file mode 100644
index 0000000000000000000000000000000000000000..138641665c1ea792c182eb11e2473831c3ae39f0
--- /dev/null
+++ b/base/task_scheduler/worker_thread.h
@@ -0,0 +1,103 @@
+// 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_WORKER_THREAD_H_
+#define BASE_TASK_SCHEDULER_WORKER_THREAD_H_
+
+#include "base/atomicops.h"
+#include "base/base_export.h"
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/task_scheduler/sequence.h"
+#include "base/threading/platform_thread.h"
+
+namespace base {
+namespace internal {
+
+class TaskTracker;
+
+// A thread that runs Tasks from Sequences returned by a callback.
+//
+// A WorkerThread is woken up when its WakeUp() method is called. After a wake-
+// up, a WorkerThread runs Tasks from Sequences returned by its "get work"
+// callback 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 WorkerThread : public PlatformThread::Delegate {

[danakj, 2016/03/31 00:25:13]

Did you know there's already a class named WorkerThread in base (inside a test
only)
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/wor...

As well as quite a number of other "WorkerThread" classes in chrome.
https://code.google.com/p/chromium/codesearch#search/&q=class%5C%20(.*%5C%20)...

Perhaps this could have a more unique name? SchedulerWorkerThread maybe? I'm not
very creative with names.

[fdoray, 2016/03/31 03:26:31]

Done. Renamed to SchedulerWorkerThread.

[fdoray, 2016/03/31 13:29:24]

Another solution would be to have a base::task_scheduler:: namespace.

[danakj, 2016/03/31 22:33:49]

I prefer changing the class name as you did.

+ public:
+  // Callback invoked to get a Sequence from which to run a Task on
+  // |worker_thread|.
+  using GetWorkCallback =
+      Callback<scoped_refptr<Sequence>(const WorkerThread* worker_thread)>;
+
+  // Callback invoked after |worker_thread| has tried to run a Task from
+  // |sequence| (a TaskTracker might have prevented the Task from running).
+  using RanTaskFromSequenceCallback =
+      Callback<void(const WorkerThread* worker_thread,
+                    scoped_refptr<Sequence> sequence)>;
+
+  // Creates a WorkerThread with priority |thread_priority| that runs Tasks from
+  // Sequences returned by |get_work_callback|.
+  // |ran_task_from_sequence_callback| is invoked after the WorkerThread has
+  // tried to run a Task from a Sequence returned by |get_work_callback|.
+  // |task_tracker| is used to handle shutdown behavior of Tasks.
+  static scoped_ptr<WorkerThread> CreateWorkerThread(

[danakj, 2016/03/31 00:25:13]

When can this return null? Please document.

[fdoray, 2016/03/31 03:26:31]

Done.

+      ThreadPriority thread_priority,
+      const GetWorkCallback& get_work_callback,
+      const RanTaskFromSequenceCallback& ran_task_from_sequence_callback,
+      TaskTracker* task_tracker);
+
+  // Destroying a WorkerThread in production is not allowed; it is always
+  // leaked. In tests, it can only be destroyed after JoinForTesting() has
+  // returned.
+  ~WorkerThread() override;
+
+  // Wakes up this WorkerThread. After this is called, this WorkerThread will
+  // run Tasks from Sequences returned by |get_work_callback_| until it returns
+  // nullptr.
+  void WakeUp();
+
+  // Joins this WorkerThread. If a Task is already running, it will be allowed
+  // to complete its execution. This can only be called once.
+  void JoinForTesting();
+
+ private:
+  WorkerThread(
+      ThreadPriority thread_priority,
+      const GetWorkCallback& get_work_callback,
+      const RanTaskFromSequenceCallback& ran_task_from_sequence_callback,
+      TaskTracker* task_tracker);
+
+  // PlatformThread::Delegate:
+  void ThreadMain() override;
+
+  bool should_exit_for_testing() const {
+    base::subtle::MemoryBarrier();

[danakj, 2016/03/31 00:25:13]

uhmm.. I never expected to see this. :P Why aren't you just using Lock? Maybe
you want some #ifdef UNIT_TEST?

Or, volatile?

[fdoray, 2016/03/31 03:26:31]

I wanted to avoid taking a lock in production code to check a flag that is only
used in tests.

#ifdef UNIT_TEST doesn't work because UNIT_TEST is only defined for test source
files (i.e. it is never defined when worker_thread.cc is compiled).

volatile doesn't work because it doesn't provide memory ordering. I couldn't use
it to guarantee that |should_exit_for_testing_| has been written before
JoinForTesting() signals |wake_up_event_| [1] or to guarantee that
|should_exit_for_testing_| is read at the expected time in
WorkerThread::ThreadMain [2].

I changed the code to use a lock. There won't be contention on this lock in
production code so it shouldn't be to costly...

[1] Actually, I believe that signaling |wake_up_event_| acts like a memory
barrier. However, I prefer not to rely on the side effects of signaling an
event.
[2] task_tracker_->shutdown_completed() acts like a memory barrier because it
acquires a lock. Again, I prefer not to rely on this side effect.

+    return should_exit_for_testing_;
+  }
+
+  // Platform thread managed by this WorkerThread.
+  PlatformThreadHandle thread_handle_;
+
+  // Event signaled to wake up this WorkerThread.
+  WaitableEvent wake_up_event_;
+
+  const GetWorkCallback get_work_callback_;
+  const RanTaskFromSequenceCallback ran_task_from_sequence_callback_;
+  TaskTracker* const task_tracker_;
+
+  // True once JoinForTesting() has been called.
+  bool should_exit_for_testing_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(WorkerThread);
+};
+
+}  // namespace internal
+}  // namespace base
+
+#endif  // BASE_TASK_SCHEDULER_WORKER_THREAD_H_