TaskScheduler [13] TaskSchedulerImpl

base/task_scheduler/task_scheduler_impl.cc

Index: base/task_scheduler/task_scheduler_impl.cc
diff --git a/base/task_scheduler/task_scheduler_impl.cc b/base/task_scheduler/task_scheduler_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..934261cccae8352e49dfa4a3b2cec247ea503903
--- /dev/null
+++ b/base/task_scheduler/task_scheduler_impl.cc
@@ -0,0 +1,117 @@
+// 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.
+
+#include "base/task_scheduler/task_scheduler_impl.h"
+
+#include <utility>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/logging.h"
+#include "base/task_scheduler/sequence_sort_key.h"
+
+namespace base {
+namespace internal {
+
+TaskSchedulerImpl::~TaskSchedulerImpl() {
+  DCHECK(join_for_testing_returned_.IsSignaled());
+}
+
+// static
+std::unique_ptr<TaskSchedulerImpl> TaskSchedulerImpl::Create() {

[gab, 2016/04/27 19:15:28]

In SchedulerThreadPool we had a static Create() with private constructor to
support return failure. This class doesn't support creation failure so I think
we should just use a public constructor with no Create().

[fdoray, 2016/04/28 18:36:32]

Done.

[robliao, 2016/04/29 18:34:21]

While this class doesn't support creation failure, we could leave it up to the
caller to perform the CHECK instead of doing it here.

[fdoray, 2016/04/29 19:42:42]

If TaskSchedulerImpl::Create successfully creates the 3 first thread pools of a
TaskSchedulerImpl but fails to create the last one, it needs to delete the 3
first thread pools before returning nullptr. Unfortunately, we currently don't
support deleting a thread pool without calling its JoinForTesting() method
first. I think a CHECK() as soon as we fail to create a thread pool is better
than supporting deleting a thread pool in production.

[gab, 2016/04/29 20:33:18]

Agreed, and I think the "doing work in the constructor" rule is meant to address
inability to return failure or exceptions but here we don't want to do that
anyways.

+  std::unique_ptr<TaskSchedulerImpl> scheduler(new TaskSchedulerImpl);
+  scheduler->Initialize();
+  return scheduler;
+}
+
+scoped_refptr<TaskRunner> TaskSchedulerImpl::CreateTaskRunnerWithTraits(
+    const TaskTraits& traits,
+    ExecutionMode execution_mode) {
+  return GetThreadPoolForTraits(traits)->CreateTaskRunnerWithTraits(
+      traits, execution_mode);
+}
+
+void TaskSchedulerImpl::Shutdown() {
+  // TODO(fdoray): Increase the priority of BACKGROUND tasks blocking shutdown.
+  task_tracker_.Shutdown();
+}
+
+void TaskSchedulerImpl::JoinForTesting() {
+  DCHECK(!join_for_testing_returned_.IsSignaled());
+  background_thread_pool_->JoinForTesting();
+  background_file_io_thread_pool_->JoinForTesting();
+  normal_thread_pool_->JoinForTesting();
+  normal_file_io_thread_pool_->JoinForTesting();
+  join_for_testing_returned_.Signal();
+}
+
+TaskSchedulerImpl::TaskSchedulerImpl()
+    // TODO(robliao): Wake up the service thread instead of calling DoNothing()
+    // when the delayed run time changes.
+    : delayed_task_manager_(Bind(&DoNothing)),
+      join_for_testing_returned_(true, false) {}
+
+void TaskSchedulerImpl::Initialize() {
+  const SchedulerThreadPoolImpl::ReEnqueueSequenceCallback
+      re_enqueue_sequence_callback =
+          Bind(&TaskSchedulerImpl::ReEnqueueSequenceCallback, Unretained(this));
+
+  // TODO(fdoray): Derive the number of threads per pool from hardware
+  // characteristics rather than using hard-coded constants.
+
+  // Passing pointers to objects owned by |this| to
+  // SchedulerThreadPoolImpl::Create() is safe because a TaskSchedulerImpl can't
+  // be deleted before all its thread pools have been joined.
+  background_thread_pool_ = SchedulerThreadPoolImpl::Create(
+      ThreadPriority::BACKGROUND, 1U, re_enqueue_sequence_callback,
+      &task_tracker_, &delayed_task_manager_);
+  CHECK(background_thread_pool_);
+
+  background_file_io_thread_pool_ = SchedulerThreadPoolImpl::Create(
+      ThreadPriority::BACKGROUND, 1U, re_enqueue_sequence_callback,
+      &task_tracker_, &delayed_task_manager_);
+  CHECK(background_file_io_thread_pool_);
+
+  normal_thread_pool_ = SchedulerThreadPoolImpl::Create(
+      ThreadPriority::NORMAL, 4U, re_enqueue_sequence_callback, &task_tracker_,
+      &delayed_task_manager_);
+  CHECK(normal_thread_pool_);
+
+  normal_file_io_thread_pool_ = SchedulerThreadPoolImpl::Create(
+      ThreadPriority::NORMAL, 12U, re_enqueue_sequence_callback, &task_tracker_,
+      &delayed_task_manager_);
+  CHECK(normal_file_io_thread_pool_);
+}
+
+SchedulerThreadPool* TaskSchedulerImpl::GetThreadPoolForTraits(
+    const TaskTraits& traits) {
+  if (traits.with_file_io()) {
+    if (traits.priority() == TaskPriority::BACKGROUND)
+      return background_file_io_thread_pool_.get();
+    return normal_file_io_thread_pool_.get();
+  }
+
+  if (traits.priority() == TaskPriority::BACKGROUND)
+    return background_thread_pool_.get();
+  return normal_thread_pool_.get();
+}
+
+void TaskSchedulerImpl::ReEnqueueSequenceCallback(
+    scoped_refptr<Sequence> sequence) {
+  DCHECK(sequence);
+
+  const SequenceSortKey sort_key = sequence->GetSortKey();
+  const Task* next_task_in_sequence = sequence->PeekTask();
+  DCHECK(next_task_in_sequence);
+
+  TaskTraits traits = TaskTraits().WithPriority(sort_key.priority);
+  if (next_task_in_sequence->traits.with_file_io())
+    traits = traits.WithFileIO();
+
+  GetThreadPoolForTraits(traits)->ReEnqueueSequence(std::move(sequence),

[gab, 2016/04/27 19:15:28]

Why not use |next_task_in_sequence->traits| instead of creating a new |traits|
here? Feels weird to simulate just the required traits (i.e. this code will need
to change if GetThreadPoolForTraits()'s decision ever evolves to depending on
more traits..

I would see this entire method as :

const SequenceSortKey sort_key = sequence->GetSortKey();
GetThreadPoolForTraits(sequence->PeekTask().traits)
    ->ReEnqueueSequence(std::move(sequence), sort_key);

[fdoray, 2016/04/28 18:36:32]

Added comment to explain why I do this.

+                                                    sort_key);
+}
+
+}  // namespace internal
+}  // namespace base