Remove the |max_threads| argument from CreateAndSetSimpleTaskScheduler().

Remove the |max_threads| argument from CreateAndSetSimpleTaskScheduler().

We want to change TaskSchedulerImpl so that it always has 4 thread pools. This
is incompatible with a CreateAndSetSimpleTaskScheduler() function that takes
a global maximum number of threads as argument. Therefore, this CL removes
that argument. Since existing callers didn't have comments to justify how the
value of this argument was chosen, they shouldn't care too much about this
change.

This CL also adds a |name| argument to CreateAndSetSimpleTaskScheduler()
that is used to label threads and histograms associated with the created
TaskScheduler.

BUG=690706
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2729523006
Cr-Commit-Position: refs/heads/master@{#458450}
Committed: https://chromium.googlesource.com/chromium/src/+/dafc3415b9ca6afe4c08690891e6331280ee7bbb

[fdoray, 2017-03-13 22:07:37]

Description was changed from

==========
Remove the |max_threads| argument from CreateAndSetSimpleTaskScheduler().

BUG=
==========

to

==========
Remove the |max_threads| argument from CreateAndSetSimpleTaskScheduler().

We want to change TaskSchedulerImpl so that it always has 4 thread pools. This
is incompatible with a CreateAndSetSimpleTaskScheduler() function that takes
a global maximum number of threads as argument.

Also, existing CreateAndSetSimpleTaskScheduler() call sites do not justify
their choice of |max_threads|. We believe that having the logic to choose the
maximum number of threads for all "simple" TaskSchedulers in one place is
better than spreading different constants/algorithms across the codebase.

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

[fdoray, 2017-03-13 22:09:03]

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

[fdoray, 2017-03-13 22:09:14]

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

[fdoray, 2017-03-13 22:09:15]

PTAL

[commit-bot: I haz the power, 2017-03-13 22:09:42]

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-03-13 22:16:53]

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

[commit-bot: I haz the power, 2017-03-13 22:16:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_...)

[robliao, 2017-03-13 23:56:05]

Looks generally good. Some comments and questions.

https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler.cc (right):

https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler.cc:28: constexpr int kMinNumThreads = 2;
For single-core non-hyperthreaded processors, should this be 2?

https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler.h (right):

https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler.h:112: // to label threads and histograms.
CHECKs on failure. For tests, prefer
Add guidance on choosing a name (e.g. it should identify your component).

https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler.h:114: static void
CreateAndSetSimpleTaskScheduler(const std::string& name);
Note this change in the CL description.

https://codereview.chromium.org/2729523006/diff/40001/chrome/service/service_...
File chrome/service/service_process.cc (right):

https://codereview.chromium.org/2729523006/diff/40001/chrome/service/service_...
chrome/service/service_process.cc:164:
base::TaskScheduler::CreateAndSetSimpleTaskScheduler("Service");
Maybe CloudPrintServiceProcess?

https://codereview.chromium.org/2729523006/diff/40001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.mm (right):

https://codereview.chromium.org/2729523006/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.mm:121:
base::TaskScheduler::CreateAndSetSimpleTaskScheduler("Cronet");
CronetIos might be more specific.

Remoting in a later one is shared between iOS and others. Should it be shared?

[fdoray, 2017-03-14 14:55:31]

Description was changed from

==========
Remove the |max_threads| argument from CreateAndSetSimpleTaskScheduler().

We want to change TaskSchedulerImpl so that it always has 4 thread pools. This
is incompatible with a CreateAndSetSimpleTaskScheduler() function that takes
a global maximum number of threads as argument.

Also, existing CreateAndSetSimpleTaskScheduler() call sites do not justify
their choice of |max_threads|. We believe that having the logic to choose the
maximum number of threads for all "simple" TaskSchedulers in one place is
better than spreading different constants/algorithms across the codebase.

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

to

==========
Remove the |max_threads| argument from CreateAndSetSimpleTaskScheduler().

We want to change TaskSchedulerImpl so that it always has 4 thread pools. This
is incompatible with a CreateAndSetSimpleTaskScheduler() function that takes
a global maximum number of threads as argument. Therefore, this CL removes
that argument. Since existing callers didn't have comments to justify how the
value of this argument was chosen, they shouldn't care too much about this
change.

This CL also adds a |name| argument to CreateAndSetSimpleTaskScheduler()
that is used to label threads and histograms associated with the created
TaskScheduler.

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

[fdoray, 2017-03-14 15:08:22]

PTAnL

https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler.cc (right):

https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler.cc:28: constexpr int kMinNumThreads = 2;
On 2017/03/13 23:56:05, robliao wrote:
> For single-core non-hyperthreaded processors, should this be 2?

1 might be a more justifiable lower bound.

https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler.h (right):

https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler.h:112: // to label threads and histograms.
CHECKs on failure. For tests, prefer
On 2017/03/13 23:56:05, robliao wrote:
> Add guidance on choosing a name (e.g. it should identify your component).

Done.

https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler.h:114: static void
CreateAndSetSimpleTaskScheduler(const std::string& name);
On 2017/03/13 23:56:05, robliao wrote:
> Note this change in the CL description.

Done.

https://codereview.chromium.org/2729523006/diff/40001/chrome/service/service_...
File chrome/service/service_process.cc (right):

https://codereview.chromium.org/2729523006/diff/40001/chrome/service/service_...
chrome/service/service_process.cc:164:
base::TaskScheduler::CreateAndSetSimpleTaskScheduler("Service");
On 2017/03/13 23:56:05, robliao wrote:
> Maybe CloudPrintServiceProcess?

Done.

https://codereview.chromium.org/2729523006/diff/40001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.mm (right):

https://codereview.chromium.org/2729523006/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.mm:121:
base::TaskScheduler::CreateAndSetSimpleTaskScheduler("Cronet");
On 2017/03/13 23:56:05, robliao wrote:
> CronetIos might be more specific.
> 
> Remoting in a later one is shared between iOS and others. Should it be shared?

Done.

[robliao, 2017-03-14 20:06:09]

lgtm + nit. Thanks!

https://codereview.chromium.org/2729523006/diff/80001/content/child/child_pro...
File content/child/child_process.cc (right):

https://codereview.chromium.org/2729523006/diff/80001/content/child/child_pro...
content/child/child_process.cc:59:
base::TaskScheduler::CreateAndSetSimpleTaskScheduler("Child");
Nit: ContentChildProcess might be more specific.

[fdoray, 2017-03-15 14:24:19]

https://codereview.chromium.org/2729523006/diff/80001/content/child/child_pro...
File content/child/child_process.cc (right):

https://codereview.chromium.org/2729523006/diff/80001/content/child/child_pro...
content/child/child_process.cc:59:
base::TaskScheduler::CreateAndSetSimpleTaskScheduler("Child");
On 2017/03/14 20:06:08, robliao wrote:
> Nit: ContentChildProcess might be more specific.

Done.

[fdoray, 2017-03-15 15:16:25]

fdoray@chromium.org changed reviewers:
+ jam@chromium.org, pauljensen@chromium.org, sergeyu@chromium.org,
sky@chromium.org

[fdoray, 2017-03-15 15:16:26]

pauljensen@chromium.org: Please review changes in components/cronet/

sky@chromium.org: Please review changes in chrome/

sergeyu@chromium.org: Please review changes in remoting/

jam@chromium.org: Please review changes in content/

[pauljensen, 2017-03-15 15:40:12]

pauljensen@chromium.org changed reviewers:
+ kapishnikov@chromium.org, lilyhoughton@chromium.org

[pauljensen, 2017-03-15 15:40:13]

Andrei (who is familiar with Cronet iOS and an OWNER) and Lily (who wrote the
code being modified), PTAL @ components/cronet/
Moving myself to CC.

[jam, 2017-03-15 17:07:51]

lgtm

[kapishnikov, 2017-03-15 18:28:02]

LGTM for Cronet

[gab, 2017-03-15 19:34:13]

lgtm

[sky, 2017-03-15 19:53:05]

mash_runner LGTM - fdoray, as you changed chrome/service/service_process.cc I'm
assuming you know the change to 3 is ok.

[nicholss, 2017-03-17 17:20:54]

nicholss@chromium.org changed reviewers:
+ nicholss@chromium.org

[nicholss, 2017-03-17 17:20:55]

There will be  a merge conflict, heads up.

https://codereview.chromium.org/2729523006/diff/100001/remoting/client/jni/ch...
File remoting/client/jni/chromoting_jni_runtime.cc (right):

https://codereview.chromium.org/2729523006/diff/100001/remoting/client/jni/ch...
remoting/client/jni/chromoting_jni_runtime.cc:46:
base::TaskScheduler::CreateAndSetSimpleTaskScheduler("RemotingJava");
I have removed this file in https://codereview.chromium.org/2745583008/

It is now located: remoting/client/chromoting_client_runtime.cc

[fdoray, 2017-03-20 14:52:13]

fdoray@chromium.org changed reviewers:
- pauljensen@chromium.org

[fdoray, 2017-03-20 16:58:41]

ping sergeyu@

https://codereview.chromium.org/2729523006/diff/100001/remoting/client/jni/ch...
File remoting/client/jni/chromoting_jni_runtime.cc (right):

https://codereview.chromium.org/2729523006/diff/100001/remoting/client/jni/ch...
remoting/client/jni/chromoting_jni_runtime.cc:46:
base::TaskScheduler::CreateAndSetSimpleTaskScheduler("RemotingJava");
On 2017/03/17 17:20:54, nicholss wrote:
> I have removed this file in https://codereview.chromium.org/2745583008/
> 
> It is now located: remoting/client/chromoting_client_runtime.cc

Done.

[Sergey Ulanov, 2017-03-20 17:45:20]

As far as I understand there can't be more than one TaskScheduler per process
and CreateAndSetSimpleTaskScheduler() won't be called more than once in the same
process. So I'm not sure what's the significance of the name parameter that call
takes.
Otherwise LGTM.

[fdoray, 2017-03-20 18:02:46]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-03-20 18:02:47]

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

[fdoray, 2017-03-20 18:03:33]

On 2017/03/20 17:45:20, Sergey Ulanov wrote:
> As far as I understand there can't be more than one TaskScheduler per process
> and CreateAndSetSimpleTaskScheduler() won't be called more than once in the
same
> process. So I'm not sure what's the significance of the name parameter that
call
> takes.
> Otherwise LGTM.

The parameter is useful to distinguish histograms recorded from different
process types.

[commit-bot: I haz the power, 2017-03-20 18:04:31]

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-03-20 18:20:39]

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

[commit-bot: I haz the power, 2017-03-20 18:20:41]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  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-03-20 19:07:15]

Description was changed from

==========
Remove the |max_threads| argument from CreateAndSetSimpleTaskScheduler().

We want to change TaskSchedulerImpl so that it always has 4 thread pools. This
is incompatible with a CreateAndSetSimpleTaskScheduler() function that takes
a global maximum number of threads as argument. Therefore, this CL removes
that argument. Since existing callers didn't have comments to justify how the
value of this argument was chosen, they shouldn't care too much about this
change.

This CL also adds a |name| argument to CreateAndSetSimpleTaskScheduler()
that is used to label threads and histograms associated with the created
TaskScheduler.

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

to

==========
Remove the |max_threads| argument from CreateAndSetSimpleTaskScheduler().

We want to change TaskSchedulerImpl so that it always has 4 thread pools. This
is incompatible with a CreateAndSetSimpleTaskScheduler() function that takes
a global maximum number of threads as argument. Therefore, this CL removes
that argument. Since existing callers didn't have comments to justify how the
value of this argument was chosen, they shouldn't care too much about this
change.

This CL also adds a |name| argument to CreateAndSetSimpleTaskScheduler()
that is used to label threads and histograms associated with the created
TaskScheduler.

BUG=690706
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[fdoray, 2017-03-20 19:07:23]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-03-20 19:07:24]

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

[commit-bot: I haz the power, 2017-03-20 19:08:32]

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-03-20 19:20:27]

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

[commit-bot: I haz the power, 2017-03-20 19:20:29]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2017-03-20 19:38:40]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-03-20 19:39:11]

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-03-20 20:04:57]

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

[commit-bot: I haz the power, 2017-03-20 20:04:59]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2017-03-20 20:11:20]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-03-20 20:12:03]

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-03-20 20:27:59]

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

[commit-bot: I haz the power, 2017-03-20 20:28:00]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2017-03-20 20:40:20]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-03-20 20:40:20]

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

[commit-bot: I haz the power, 2017-03-20 20:40:51]

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-03-20 21:12:28]

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

[commit-bot: I haz the power, 2017-03-20 21:12:30]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2017-03-21 14:27:40]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-03-21 14:28:06]

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-03-21 14:49:05]

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

[commit-bot: I haz the power, 2017-03-21 14:49:06]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2017-03-21 16:06:35]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-03-21 16:06:36]

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

[commit-bot: I haz the power, 2017-03-21 16:06:57]

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-03-21 17:19:33]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490112395438170,
"parent_rev": "3eaf76959f94c50fd558ca7cc1a29a4241b2c662", "commit_rev":
"dafc3415b9ca6afe4c08690891e6331280ee7bbb"}

[commit-bot: I haz the power, 2017-03-21 17:20:13]

Description was changed from

==========
Remove the |max_threads| argument from CreateAndSetSimpleTaskScheduler().

We want to change TaskSchedulerImpl so that it always has 4 thread pools. This
is incompatible with a CreateAndSetSimpleTaskScheduler() function that takes
a global maximum number of threads as argument. Therefore, this CL removes
that argument. Since existing callers didn't have comments to justify how the
value of this argument was chosen, they shouldn't care too much about this
change.

This CL also adds a |name| argument to CreateAndSetSimpleTaskScheduler()
that is used to label threads and histograms associated with the created
TaskScheduler.

BUG=690706
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Remove the |max_threads| argument from CreateAndSetSimpleTaskScheduler().

We want to change TaskSchedulerImpl so that it always has 4 thread pools. This
is incompatible with a CreateAndSetSimpleTaskScheduler() function that takes
a global maximum number of threads as argument. Therefore, this CL removes
that argument. Since existing callers didn't have comments to justify how the
value of this argument was chosen, they shouldn't care too much about this
change.

This CL also adds a |name| argument to CreateAndSetSimpleTaskScheduler()
that is used to label threads and histograms associated with the created
TaskScheduler.

BUG=690706
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

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

[commit-bot: I haz the power, 2017-03-21 17:20:15]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/dafc3415b9ca6afe4c08690891e6...