Always use TaskScheduler::InitParams to initialize a TaskScheduler.

Always use TaskScheduler::InitParams to initialize a TaskScheduler.

This CL removes code that allows initialization of TaskSchedulerImp
with a vector of SchedulerWorkerPoolParams and a TaskTraits->pool
mapping function. TaskSchedulerImpl is now always initialized with a
TaskScheduler::InitParams, always has four pools and has a hard-coded
TaskTraits->pool mapping function.

BUG=690706
Review-Url: https://codereview.chromium.org/2809163003
Cr-Commit-Position: refs/heads/master@{#465583}
Committed: https://chromium.googlesource.com/chromium/src/+/e7e8477007b26a23de2a945ec69d11e766f3fb70

[fdoray, 2017-04-11 16:17:00]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[fdoray, 2017-04-11 16:17:15]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org, robliao@chromium.org

[fdoray, 2017-04-11 16:17:16]

PTAL

[commit-bot: I haz the power, 2017-04-11 16:17:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 16:21:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 16:21:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fdoray, 2017-04-11 17:19:15]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 17:20:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 17:23:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 17:23:46]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fdoray, 2017-04-11 17:43:31]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 17:44:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2017-04-11 17:46:11]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 17:46:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 17:51:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 17:51:42]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gab, 2017-04-12 18:46:25]

lgtm w/ comments and happy bots :)

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler_impl.h (right):

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler_impl.h:73: explicit TaskSchedulerImpl(const
std::string& name);
All of these string params could be StringPieces to avoid copying the
char-buffer into a temp string which is then copied into the impl's string?

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler_impl_unittest.cc:286: }*/
Fix temp code?

[robliao, 2017-04-12 19:08:13]

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right):

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:403:
StringPrintf("TaskScheduler%sSingleThread%d", name.c_str(), id));
This naming makes it harder to sort against all of the single threaded workers.
This is why I kept %s at the end instead of the middle.

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc
(left):

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:186:
FROM_HERE, Bind(&CaptureThreadPriority, &thread_priority_user_blocking));
What's the rationale for removing this test?

[fdoray, 2017-04-12 19:54:35]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 19:55:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2017-04-12 20:00:23]

PTAnL

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right):

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:403:
StringPrintf("TaskScheduler%sSingleThread%d", name.c_str(), id));
On 2017/04/12 19:08:13, robliao wrote:
> This naming makes it harder to sort against all of the single threaded
workers.
> This is why I kept %s at the end instead of the middle.

Done.

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc
(left):

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:186:
FROM_HERE, Bind(&CaptureThreadPriority, &thread_priority_user_blocking));
On 2017/04/12 19:08:13, robliao wrote:
> What's the rationale for removing this test?

Since SchedulerSingleThreadTaskRunnerManager doesn't look at TaskPriority, I
don't think it makes sense to have a test for each TaskPriority.
SchedulerSingleThreadTaskRunnerManager only cares about ThreadPriority.

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler_impl.h (right):

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler_impl.h:73: explicit TaskSchedulerImpl(const
std::string& name);
On 2017/04/12 18:46:24, gab wrote:
> All of these string params could be StringPieces to avoid copying the
> char-buffer into a temp string which is then copied into the impl's string?

Keeping this change for a separate CL since it involve changing methods not
touched by this CL. https://codereview.chromium.org/2817523004

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler_impl_unittest.cc:286: }*/
On 2017/04/12 18:46:24, gab wrote:
> Fix temp code?

Done.

[robliao, 2017-04-12 20:37:50]

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right):

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:403:
StringPrintf("TaskScheduler%sSingleThread%d", name.c_str(), id));
On 2017/04/12 20:00:23, fdoray wrote:
> On 2017/04/12 19:08:13, robliao wrote:
> > This naming makes it harder to sort against all of the single threaded
> workers.
> > This is why I kept %s at the end instead of the middle.
> 
> Done.

This leads me to my second question. It does seem strange that the caller
specifies both the priority and the traits to create a SingleThreadTaskRunner.
Priority is generally derived from the traits and that policy is set by the
TaskSchedulerImpl. Previously, SchedulerSingleThreadTaskRunnerManager accepted
that policy determination from TaskSchedulerImpl to determine the priority here.
Would a similar callback make sense here too?

[fdoray, 2017-04-12 20:57:48]

Ptanl

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right):

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:403:
StringPrintf("TaskScheduler%sSingleThread%d", name.c_str(), id));
On 2017/04/12 20:37:50, robliao wrote:
> On 2017/04/12 20:00:23, fdoray wrote:
> > On 2017/04/12 19:08:13, robliao wrote:
> > > This naming makes it harder to sort against all of the single threaded
> > workers.
> > > This is why I kept %s at the end instead of the middle.
> > 
> > Done.
> 
> This leads me to my second question. It does seem strange that the caller
> specifies both the priority and the traits to create a SingleThreadTaskRunner.
> Priority is generally derived from the traits and that policy is set by the
> TaskSchedulerImpl. Previously, SchedulerSingleThreadTaskRunnerManager accepted
> that policy determination from TaskSchedulerImpl to determine the priority
here.
> Would a similar callback make sense here too?

In this CL, TaskSchedulerImpl derives the thread priority from TaskTraits and
provides it directly to SchedulerSingleThreadTaskRunnerManager instead of
providing a callback+vector<SchedulerWorkerPoolParams>.

[commit-bot: I haz the power, 2017-04-12 21:07:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 21:07:40]

Dry run: This issue passed the CQ dry run.

[robliao, 2017-04-12 21:09:05]

lgtm + comments

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right):

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:403:
StringPrintf("TaskScheduler%sSingleThread%d", name.c_str(), id));
On 2017/04/12 20:57:48, fdoray wrote:
> On 2017/04/12 20:37:50, robliao wrote:
> > On 2017/04/12 20:00:23, fdoray wrote:
> > > On 2017/04/12 19:08:13, robliao wrote:
> > > > This naming makes it harder to sort against all of the single threaded
> > > workers.
> > > > This is why I kept %s at the end instead of the middle.
> > > 
> > > Done.
> > 
> > This leads me to my second question. It does seem strange that the caller
> > specifies both the priority and the traits to create a
SingleThreadTaskRunner.
> > Priority is generally derived from the traits and that policy is set by the
> > TaskSchedulerImpl. Previously, SchedulerSingleThreadTaskRunnerManager
accepted
> > that policy determination from TaskSchedulerImpl to determine the priority
> here.
> > Would a similar callback make sense here too?
> 
> In this CL, TaskSchedulerImpl derives the thread priority from TaskTraits and
> provides it directly to SchedulerSingleThreadTaskRunnerManager instead of
> providing a callback+vector<SchedulerWorkerPoolParams>. 

Right. This also means that later on someone could pass
ThreadPriority::BACKGROUND with
TaskTraits().WithPriority(TaskPriority::USER_BLOCKING), which definitely seems
wrong.

If it makes this CL easier to land, I'm fine with this shortcoming, but would
just like to voice my objection to this signature and think the right thing to
do is have this call back to TaskSchedulerImpl for the priority instead of
receiving it from TaskSchedulerImpl.

https://codereview.chromium.org/2809163003/diff/100001/base/task_scheduler/ta...
File base/task_scheduler/task_scheduler_impl.h (right):

https://codereview.chromium.org/2809163003/diff/100001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl.h:91:
std::unique_ptr<SchedulerWorkerPoolImpl> worker_pools_[4];
Nit: Document how we got 4.

[fdoray, 2017-04-19 12:21:21]

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right):

https://codereview.chromium.org/2809163003/diff/80001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:403:
StringPrintf("TaskScheduler%sSingleThread%d", name.c_str(), id));
On 2017/04/12 21:09:05, robliao wrote:
> On 2017/04/12 20:57:48, fdoray wrote:
> > On 2017/04/12 20:37:50, robliao wrote:
> > > On 2017/04/12 20:00:23, fdoray wrote:
> > > > On 2017/04/12 19:08:13, robliao wrote:
> > > > > This naming makes it harder to sort against all of the single threaded
> > > > workers.
> > > > > This is why I kept %s at the end instead of the middle.
> > > > 
> > > > Done.
> > > 
> > > This leads me to my second question. It does seem strange that the caller
> > > specifies both the priority and the traits to create a
> SingleThreadTaskRunner.
> > > Priority is generally derived from the traits and that policy is set by
the
> > > TaskSchedulerImpl. Previously, SchedulerSingleThreadTaskRunnerManager
> accepted
> > > that policy determination from TaskSchedulerImpl to determine the priority
> > here.
> > > Would a similar callback make sense here too?
> > 
> > In this CL, TaskSchedulerImpl derives the thread priority from TaskTraits
and
> > provides it directly to SchedulerSingleThreadTaskRunnerManager instead of
> > providing a callback+vector<SchedulerWorkerPoolParams>. 
> 
> Right. This also means that later on someone could pass
> ThreadPriority::BACKGROUND with
> TaskTraits().WithPriority(TaskPriority::USER_BLOCKING), which definitely seems
> wrong.
> 
> If it makes this CL easier to land, I'm fine with this shortcoming, but would
> just like to voice my objection to this signature and think the right thing to
> do is have this call back to TaskSchedulerImpl for the priority instead of
> receiving it from TaskSchedulerImpl.
> 

Acknowledged.

https://codereview.chromium.org/2809163003/diff/100001/base/task_scheduler/ta...
File base/task_scheduler/task_scheduler_impl.h (right):

https://codereview.chromium.org/2809163003/diff/100001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl.h:91:
std::unique_ptr<SchedulerWorkerPoolImpl> worker_pools_[4];
On 2017/04/12 21:09:05, robliao wrote:
> Nit: Document how we got 4.

Done.

[fdoray, 2017-04-19 12:23:07]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-04-19 12:23:08]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
robliao@chromium.org
Link to the patchset: https://codereview.chromium.org/2809163003/#ps120001
(title: "rebase-and-CR-robliao-26")

[commit-bot: I haz the power, 2017-04-19 12:23:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2017-04-19 12:40:13]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 12:40:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2017-04-19 12:40:32]

The CQ bit was unchecked by fdoray@chromium.org

[fdoray, 2017-04-19 12:40:35]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-04-19 12:40:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
robliao@chromium.org, gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2809163003/#ps140001
(title: "fix-build-error")

[commit-bot: I haz the power, 2017-04-19 12:40:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 13:42:29]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492605635750390,
"parent_rev": "4f9c21746d89a089e77f5c24eb5f0dcc60d24466", "commit_rev":
"e7e8477007b26a23de2a945ec69d11e766f3fb70"}

[commit-bot: I haz the power, 2017-04-19 13:42:40]

Description was changed from

==========
Always use TaskScheduler::InitParams to initialize a TaskScheduler.

This CL removes code that allows initialization of TaskSchedulerImp
with a vector of SchedulerWorkerPoolParams and a TaskTraits->pool
mapping function. TaskSchedulerImpl is now always initialized with a
TaskScheduler::InitParams, always has four pools and has a hard-coded
TaskTraits->pool mapping function.

BUG=690706
==========

to

==========
Always use TaskScheduler::InitParams to initialize a TaskScheduler.

This CL removes code that allows initialization of TaskSchedulerImp
with a vector of SchedulerWorkerPoolParams and a TaskTraits->pool
mapping function. TaskSchedulerImpl is now always initialized with a
TaskScheduler::InitParams, always has four pools and has a hard-coded
TaskTraits->pool mapping function.

BUG=690706

Review-Url: https://codereview.chromium.org/2809163003
Cr-Commit-Position: refs/heads/master@{#465583}
Committed:
https://chromium.googlesource.com/chromium/src/+/e7e8477007b26a23de2a945ec69d...
==========

[commit-bot: I haz the power, 2017-04-19 13:42:42]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/e7e8477007b26a23de2a945ec69d...