TaskScheduler: Make the worker pools of TaskSchedulerImpl configurable

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
index e46a5b42977288fe6f9afb1c4c1b0d78c442473c..69603afcc5a14f7e8f652e4b48e8917db1a2416f 100644
--- a/base/task_scheduler/task_scheduler_impl.cc
+++ b/base/task_scheduler/task_scheduler_impl.cc
@@ -10,7 +10,6 @@
 #include "base/bind_helpers.h"
 #include "base/memory/ptr_util.h"
 #include "base/task_scheduler/scheduler_service_thread.h"
-#include "base/task_scheduler/scheduler_thread_pool_impl.h"
 #include "base/task_scheduler/sequence_sort_key.h"
 #include "base/task_scheduler/task.h"
 #include "base/time/time.h"
@@ -19,8 +18,10 @@ namespace base {
 namespace internal {
 
 // static
-std::unique_ptr<TaskSchedulerImpl> TaskSchedulerImpl::Create() {
-  std::unique_ptr<TaskSchedulerImpl> scheduler(new TaskSchedulerImpl);
+std::unique_ptr<TaskSchedulerImpl> TaskSchedulerImpl::Create(
+    std::unique_ptr<Delegate> delegate) {
+  std::unique_ptr<TaskSchedulerImpl> scheduler(
+      new TaskSchedulerImpl(std::move(delegate)));
   scheduler->Initialize();
   return scheduler;
 }
@@ -57,18 +58,17 @@ void TaskSchedulerImpl::JoinForTesting() {
 #if DCHECK_IS_ON()
   DCHECK(!join_for_testing_returned_.IsSignaled());
 #endif
-  background_thread_pool_->JoinForTesting();
-  background_file_io_thread_pool_->JoinForTesting();
-  normal_thread_pool_->JoinForTesting();
-  normal_file_io_thread_pool_->JoinForTesting();
+  for (const auto& thread_pool : thread_pools_)
+    thread_pool->JoinForTesting();
   service_thread_->JoinForTesting();
 #if DCHECK_IS_ON()
   join_for_testing_returned_.Signal();
 #endif
 }
 
-TaskSchedulerImpl::TaskSchedulerImpl()
-    : delayed_task_manager_(
+TaskSchedulerImpl::TaskSchedulerImpl(std::unique_ptr<Delegate> delegate)
+    : delegate_(std::move(delegate)),
+      delayed_task_manager_(
           Bind(&TaskSchedulerImpl::OnDelayedRunTimeUpdated, Unretained(this)))
 #if DCHECK_IS_ON()
       ,
@@ -76,44 +76,31 @@ TaskSchedulerImpl::TaskSchedulerImpl()
                                  WaitableEvent::InitialState::NOT_SIGNALED)
 #endif
 {
+  DCHECK(delegate_);
 }
 
 void TaskSchedulerImpl::Initialize() {
-  using IORestriction = SchedulerThreadPoolImpl::IORestriction;
+  constexpr char kTaskSchedulerThreadNamePrefix[] = "TaskScheduler";
 
   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(
-      "TaskSchedulerBackground", ThreadPriority::BACKGROUND, 1U,
-      IORestriction::DISALLOWED, re_enqueue_sequence_callback, &task_tracker_,
-      &delayed_task_manager_);
-  CHECK(background_thread_pool_);
-
-  background_file_io_thread_pool_ = SchedulerThreadPoolImpl::Create(
-      "TaskSchedulerBackgroundFileIO", ThreadPriority::BACKGROUND, 1U,
-      IORestriction::ALLOWED, re_enqueue_sequence_callback, &task_tracker_,
-      &delayed_task_manager_);
-  CHECK(background_file_io_thread_pool_);
-
-  normal_thread_pool_ = SchedulerThreadPoolImpl::Create(
-      "TaskSchedulerForeground", ThreadPriority::NORMAL, 4U,
-      IORestriction::DISALLOWED, re_enqueue_sequence_callback, &task_tracker_,
-      &delayed_task_manager_);
-  CHECK(normal_thread_pool_);
-
-  normal_file_io_thread_pool_ = SchedulerThreadPoolImpl::Create(
-      "TaskSchedulerForegroundFileIO", ThreadPriority::NORMAL, 12U,
-      IORestriction::ALLOWED, re_enqueue_sequence_callback, &task_tracker_,
-      &delayed_task_manager_);
-  CHECK(normal_file_io_thread_pool_);
+  const size_t num_thread_pools = delegate_->GetNumThreadPools();

[robliao, 2016/06/16 20:51:15]

I'm not sure it's necessary to make the TaskScheduler this generic. 

Do we need the ability to reconfigure the threadpool hierarchy with finch?

If the answer for reconfiguration is no, then it may be more straightforward to
pursue a Factory pattern instead of a delegate pattern.

I anticipate that we could have two concrete implementations of
TaskSchedulerImpl instead of needing to do data directed configuration  for both
scenarios.

[fdoray, 2016/06/17 15:14:34]

We don't plan to reconfigure the thread pool hierarchy with finch. However,
having this capability makes it easier to use TaskSchedulerImpl outside the
Chrome browser process (e.g. a file i/o thread pool doesn't make sense in a
renderer).

TaskSchedulerImpl needs variation params put cannot call
variations::GetVariationParams directly (because it's in
components/variations/*). That means that code that instantiates
TaskSchedulerImpl is responsible for providing these variation params. Most
strategies to do that involve that TaskSchedulerImpl exposes the fact that it
manages 4 thread pools in its interface. With a delegate, TaskSchedulerImpl
doesn't have to commit itself to managing 4 thread pools in its interface.

A Factory that TaskSchedulerImpl would use to create its ThreadPools (e.g. "give
me a background file i/o thread pool"?) That still involves TaskSchedulerImpl
exposing the fact that it manages 4 thread pools to its users?

"two concrete implementations" = test and prod?

+  for (size_t i = 0; i < num_thread_pools; ++i) {
+    const Delegate::ThreadPoolCreationArgs creation_args =
+        delegate_->GetCreationArgsForThreadPool(i);
+
+    // 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.
+    thread_pools_.push_back(SchedulerThreadPoolImpl::Create(
+        kTaskSchedulerThreadNamePrefix + creation_args.name,
+        creation_args.thread_priority, creation_args.max_threads,
+        creation_args.io_restriction, re_enqueue_sequence_callback,
+        &task_tracker_, &delayed_task_manager_));
+    CHECK(thread_pools_.back());
+  }
 
   service_thread_ = SchedulerServiceThread::Create(&task_tracker_,
                                                    &delayed_task_manager_);
@@ -122,15 +109,9 @@ void TaskSchedulerImpl::Initialize() {
 
 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();
+  const size_t index = delegate_->GetThreadPoolIndexForTraits(traits);
+  DCHECK_LT(index, thread_pools_.size());
+  return thread_pools_[index].get();
 }
 
 void TaskSchedulerImpl::ReEnqueueSequenceCallback(