TaskScheduler: Make the worker pools of TaskSchedulerImpl configurable

base/task_scheduler/task_scheduler_impl_unittest.cc

Index: base/task_scheduler/task_scheduler_impl_unittest.cc
diff --git a/base/task_scheduler/task_scheduler_impl_unittest.cc b/base/task_scheduler/task_scheduler_impl_unittest.cc
index e79a381c350ac0d2ec5d000adabc365b90fc429b..011d4bed42dc9022bdce245ca7757849b35d6674 100644
--- a/base/task_scheduler/task_scheduler_impl_unittest.cc
+++ b/base/task_scheduler/task_scheduler_impl_unittest.cc
@@ -38,23 +38,6 @@ struct TraitsExecutionModePair {
   ExecutionMode execution_mode;
 };
 
-class TaskSchedulerImplTest
-    : public testing::TestWithParam<TraitsExecutionModePair> {
- protected:
-  TaskSchedulerImplTest() = default;
-
-  void SetUp() override {
-    scheduler_ = TaskSchedulerImpl::Create();
-    EXPECT_TRUE(scheduler_);
-  }
-  void TearDown() override { scheduler_->JoinForTesting(); }
-
-  std::unique_ptr<TaskSchedulerImpl> scheduler_;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TaskSchedulerImplTest);
-};
-
 #if ENABLE_THREAD_RESTRICTIONS
 // Returns whether I/O calls are allowed on the current thread.
 bool GetIOAllowed() {
@@ -152,6 +135,45 @@ std::vector<TraitsExecutionModePair> GetTraitsExecutionModePairs() {
   return params;
 }
 
+size_t GetThreadPoolIndexForTraits(const TaskTraits& traits) {
+  if (traits.with_file_io())
+    return traits.priority() == TaskPriority::BACKGROUND ? 1U : 3U;

[robliao, 2016/06/23 22:37:03]

Should we encourage that the return values of these be enums : size_t?

[gab, 2016/06/27 18:48:23]

I would like for implementations of this to look similar to how
TaskSchedulerImpl::GetWorkerPoolForTraits() used to look, returning raw indices
looks easy to mess up to me.

I think callers should keep constants to the index of each pool (or maybe an
enum : size_t, not sure yet) and then use names instead of raw indices here.
This will be strong versus a refactor as the names would change whereas it's
easy to forget one raw index..

[fdoray, 2016/06/27 19:45:04]

I now use an enum. Not sure that an enum class that we always cast to size_t is
better than a simple enum.

+  return traits.priority() == TaskPriority::BACKGROUND ? 0U : 2U;
+}
+
+class TaskSchedulerImplTest
+    : public testing::TestWithParam<TraitsExecutionModePair> {
+ protected:
+  TaskSchedulerImplTest() = default;
+
+  void SetUp() override {
+    using IORestriction = SchedulerWorkerPoolImpl::IORestriction;
+
+    std::vector<TaskSchedulerImpl::WorkerPoolCreationArgs>

[robliao, 2016/06/23 22:37:03]

Since the vector is set up at the get go, we can make the vector const.

[fdoray, 2016/06/27 19:45:05]

The vector can no longer be const.

[robliao, 2016/06/28 00:24:24]

An alternative is to do a post examination of the vector after construction to
keep the const. The size checking is fine too and avoids having to store the
WorkerPoolCreationArgs as a local for comparison. Your choice.

e.g.
worker_pools[BACKGROUND_WORKER_POOL] == background_worker_pool_creation_args

[fdoray, 2016/06/28 15:23:58]

I prefer not to define operator== just for this.

+        worker_pool_creation_args{

[gab, 2016/06/27 18:48:23]

Space before '{' or is no space the style for these in-place initialization
list?

[fdoray, 2016/06/27 19:45:05]

git cl format removes the space when I add it...

+            {"TaskSchedulerBackground", ThreadPriority::BACKGROUND,
+             IORestriction::DISALLOWED, 1U},
+            {"TaskSchedulerBackgroundFileIO", ThreadPriority::BACKGROUND,
+             IORestriction::ALLOWED, 3U},
+            {"TaskSchedulerForeground", ThreadPriority::NORMAL,
+             IORestriction::DISALLOWED, 4U},
+            {"TaskSchedulerForegroundFileIO", ThreadPriority::NORMAL,
+             IORestriction::ALLOWED, 12U},

[gab, 2016/06/27 18:48:23]

Reinforcing my comment above: I think impls should be written such that this
list and the indices returned from GetThreadPoolIndexForTraits() have a strong
connection (i.e. a shared constant or something).

This test needs to be the first example of how this can be written (and if it
can't then I don't think this API will do).

[fdoray, 2016/06/27 19:45:05]

Done.

+        };
+
+    scheduler_ = TaskSchedulerImpl::Create(worker_pool_creation_args,
+                                           Bind(&GetThreadPoolIndexForTraits));
+    EXPECT_TRUE(scheduler_);

[robliao, 2016/06/23 22:37:03]

This should probably be an ASSERT since the guarantee is we CHECK on failure.

[gab, 2016/06/27 18:48:23]

Agreed, plus the rest of the test is meaningless if this is null.

[fdoray, 2016/06/27 19:45:05]

Done.

+  }
+
+  void TearDown() override { scheduler_->JoinForTesting(); }
+
+  std::unique_ptr<TaskSchedulerImpl> scheduler_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TaskSchedulerImplTest);
+};
+
 }  // namespace
 
 // Verifies that a Task posted via PostTaskWithTraits with parameterized
@@ -194,13 +216,11 @@ INSTANTIATE_TEST_CASE_P(OneTraitsExecutionModePair,
 // TaskTraits and ExecutionModes. Verifies that each Task runs on a thread with
 // the expected priority and I/O restrictions and respects the characteristics
 // of its ExecutionMode.
-TEST(TaskSchedulerImplTest, MultipleTraitsExecutionModePairs) {
-  std::unique_ptr<TaskSchedulerImpl> scheduler = TaskSchedulerImpl::Create();
-
+TEST_F(TaskSchedulerImplTest, MultipleTraitsExecutionModePairs) {
   std::vector<std::unique_ptr<ThreadPostingTasks>> threads_posting_tasks;
   for (const auto& traits_execution_mode_pair : GetTraitsExecutionModePairs()) {
     threads_posting_tasks.push_back(WrapUnique(new ThreadPostingTasks(
-        scheduler.get(), traits_execution_mode_pair.traits,
+        scheduler_.get(), traits_execution_mode_pair.traits,
         traits_execution_mode_pair.execution_mode)));
     threads_posting_tasks.back()->Start();
   }
@@ -209,8 +229,6 @@ TEST(TaskSchedulerImplTest, MultipleTraitsExecutionModePairs) {
     thread->WaitForAllTasksToRun();
     thread->Join();
   }
-
-  scheduler->JoinForTesting();
 }
 
 // TODO(fdoray): Add tests with Sequences that move around worker pools once