TaskScheduler: Make the worker pools of TaskSchedulerImpl configurable

base/task_scheduler/task_scheduler_impl_unittest.cc

 // 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 <stddef.h>
 
 #include <string>
 #include <utility>
@@ skipping 20 matching lines @@
 
 struct TraitsExecutionModePair {
   TraitsExecutionModePair(const TaskTraits& traits,
                           ExecutionMode execution_mode)
       : traits(traits), execution_mode(execution_mode) {}
 
   TaskTraits traits;
   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() {
   const bool previous_value = ThreadRestrictions::SetIOAllowed(true);
   ThreadRestrictions::SetIOAllowed(previous_value);
   return previous_value;
 }
 #endif
 
 // Verify that the current thread priority and I/O restrictions are appropriate
@@ skipping 77 matching lines @@
       params.push_back(TraitsExecutionModePair(
           TaskTraits().WithPriority(priority), execution_mode));
       params.push_back(TraitsExecutionModePair(
           TaskTraits().WithPriority(priority).WithFileIO(), execution_mode));
     }
   }
 
   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
 // TaskTraits runs on a thread with the expected priority and I/O restrictions.
 // The ExecutionMode parameter is ignored by this test.
 TEST_P(TaskSchedulerImplTest, PostTaskWithTraits) {
   WaitableEvent task_ran(WaitableEvent::ResetPolicy::MANUAL,
                          WaitableEvent::InitialState::NOT_SIGNALED);
   scheduler_->PostTaskWithTraits(
       FROM_HERE, GetParam().traits,
@@ skipping 22 matching lines @@
 }
 
 INSTANTIATE_TEST_CASE_P(OneTraitsExecutionModePair,
                         TaskSchedulerImplTest,
                         ::testing::ValuesIn(GetTraitsExecutionModePairs()));
 
 // Spawns threads that simultaneously post Tasks to TaskRunners with various
 // 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();
   }
 
   for (const auto& thread : threads_posting_tasks) {
     thread->WaitForAllTasksToRun();
     thread->Join();
   }
-
-  scheduler->JoinForTesting();
 }
 
 // TODO(fdoray): Add tests with Sequences that move around worker pools once
 // child TaskRunners are supported.
 
 }  // namespace internal
 }  // namespace base