Initialize TaskScheduler in renderers.

Initialize TaskScheduler in renderers.

This CL initializes TaskScheduler in renderers using
default arguments. ContentRenderClient may provide
different arguments.

Initially, no threads will be created by TaskScheduler
in renderers because no tasks are posted to it and
pools are initialized with StandbyThreadPolicy::LAZY.

In a separate, we will start running v8 tasks in
TaskScheduler rather than WorkerPool
(https://cs.chromium.org/chromium/src/gin/v8_platform.cc?sq=package:chromium&l=70&dr=C).

BUG=664996
Committed: https://crrev.com/d2233a758df698c669341ef2e49b1ad2b23ce8da
Cr-Commit-Position: refs/heads/master@{#438202}

[fdoray, 2016-11-11 19:18:54]

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

[commit-bot: I haz the power, 2016-11-11 19:19:21]

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, 2016-11-11 19:44:40]

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

[commit-bot: I haz the power, 2016-11-11 19:44:41]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[fdoray, 2016-11-11 20:34:56]

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

[commit-bot: I haz the power, 2016-11-11 20:35:52]

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

[fdoray, 2016-11-11 20:42:09]

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

[commit-bot: I haz the power, 2016-11-11 20:43:04]

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, 2016-11-11 21:41:52]

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

[commit-bot: I haz the power, 2016-11-11 21:41:53]

Dry run: This issue passed the CQ dry run.

[fdoray, 2016-11-14 15:28:47]

Description was changed from

==========
Enable TaskScheduler in renderers.

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

to

==========
Add TaskScheduler initialization code in RenderProcessImpl.

This CL adds a ShouldInitializeTaskScheduler() method to
ContentRendererClient. When this method returns true and
Chrome doesn't run in single-process mode, RenderProcessImpl
initializes a TaskScheduler using provided arguments in its
constructor and shuts it down in its destructor.

BUG=664996
==========

[fdoray, 2016-11-14 15:28:47]

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

[fdoray, 2016-11-14 15:32:49]

PTAL

Tests pass when ContentRendererClient::ShouldInitializeTaskScheduler() returns
true (see patch set 3). I also verified that field trials (not variation params)
are set in Chrome renderers before this method is called.

[robliao, 2016-11-14 16:05:26]

https://codereview.chromium.org/2491823005/diff/60001/content/renderer/render...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/60001/content/renderer/render...
content/renderer/render_process_impl.cc:64: bool MaybeInitializeTaskScheduler()
{
What's the behavior when base linked to chrome.dll and chrome_child.dll?
How do we access chrome.dll's task scheduler?

[gab, 2016-11-14 16:16:43]

https://codereview.chromium.org/2491823005/diff/60001/content/renderer/render...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/60001/content/renderer/render...
content/renderer/render_process_impl.cc:64: bool MaybeInitializeTaskScheduler()
{
On 2016/11/14 16:05:26, robliao wrote:
> What's the behavior when base linked to chrome.dll and chrome_child.dll?
> How do we access chrome.dll's task scheduler?

Hmm? base is linked to both but any process instance is only running one or the
other? Is there a case of running both chrome.dll and chrome_child.dll in the
same process? single-process mode I guess? Then maybe the code below is
incorrect for single-process mode?

https://codereview.chromium.org/2491823005/diff/60001/content/renderer/render...
content/renderer/render_process_impl.cc:67: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
Use base::TaskScheduler::GetInstance() instead? (potentially covers other cases
we're not aware of?)

Although we probably want to know if there are more cases so maybe:

if (...GetInstance()) { 
  // TaskScheduler is not expected to already exist unless in single process
mode.
  DCHECK(..single process..); 
  return false; 
}

Note: this also allows TaskScheduler to be live in renderers and not in browser
if we ever care.

[fdoray, 2016-11-15 16:29:01]

Description was changed from

==========
Add TaskScheduler initialization code in RenderProcessImpl.

This CL adds a ShouldInitializeTaskScheduler() method to
ContentRendererClient. When this method returns true and
Chrome doesn't run in single-process mode, RenderProcessImpl
initializes a TaskScheduler using provided arguments in its
constructor and shuts it down in its destructor.

BUG=664996
==========

to

==========
Initialize TaskScheduler in renderers.

This CL initializes TaskScheduler in renderers using
arguments provided by the ContentRendererClient. By
default, no tasks are redirected to TaskScheduler.

BUG=664996
==========

[fdoray, 2016-11-15 16:30:35]

PTAnL

With this CL, TaskScheduler is always initialized in renderers. Before it lands,
we will also need to initialize TaskScheduler by default in the browser process.

https://codereview.chromium.org/2491823005/diff/60001/content/renderer/render...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/60001/content/renderer/render...
content/renderer/render_process_impl.cc:64: bool MaybeInitializeTaskScheduler()
{
On 2016/11/14 16:16:43, gab wrote:
> On 2016/11/14 16:05:26, robliao wrote:
> > What's the behavior when base linked to chrome.dll and chrome_child.dll?
> > How do we access chrome.dll's task scheduler?
> 
> Hmm? base is linked to both but any process instance is only running one or
the
> other? Is there a case of running both chrome.dll and chrome_child.dll in the
> same process? single-process mode I guess? Then maybe the code below is
> incorrect for single-process mode?

--single-process only works in component builds
https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?sq=pa...
, so it is not possible to have a process that loads chrome.dll and
chrome_child.dll.

https://codereview.chromium.org/2491823005/diff/60001/content/renderer/render...
content/renderer/render_process_impl.cc:67: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2016/11/14 16:16:43, gab wrote:
> Use base::TaskScheduler::GetInstance() instead? (potentially covers other
cases
> we're not aware of?)
> 
> Although we probably want to know if there are more cases so maybe:
> 
> if (...GetInstance()) { 
>   // TaskScheduler is not expected to already exist unless in single process
> mode.
>   DCHECK(..single process..); 
>   return false; 
> }
> 
> Note: this also allows TaskScheduler to be live in renderers and not in
browser
> if we ever care.

Done. With some changes because I don't want a RenderProcessImpl to initialize a
TaskScheduler and enable SequencedWorkerPool redirection in the browser process
(otherwise, the browser process starts without a TaskScheduler and gets one when
it launches a renderer...).

[gab, 2016-11-15 20:25:05]

lgtm % nits

(should we add the experiment hooks in ChromeContentRendererClient in this CL as
well? I think so. If you prefer to split, link to the follow-up CL in this one
to justify it?)

https://codereview.chromium.org/2491823005/diff/100001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/2491823005/diff/100001/content/public/rendere...
content/public/renderer/content_renderer_client.h:145: // repectively.
|redirect_sequenced_worker_pool| indicates whether tasks
respectively

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
content/renderer/render_process_impl.cc:74: return traits.priority() ==
base::TaskPriority::BACKGROUND
DCHECK(!traits.with_file_io());

[robliao, 2016-11-15 20:31:51]

https://codereview.chromium.org/2491823005/diff/100001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/2491823005/diff/100001/content/public/rendere...
content/public/renderer/content_renderer_client.h:142: // background pool and
foreground pool, repectively. |background_reclaim_time|
s/repectively/respectively/ and below.

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
content/renderer/render_process_impl.cc:101: DCHECK_GT(max_background_threads,
0);
Hitting this DCHECK and the one below seems like it would be pretty bad as no
workers would be allocated to create tasks.  Maybe we should CHECK instead?

[robliao, 2016-11-15 20:32:36]

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
content/renderer/render_process_impl.cc:101: DCHECK_GT(max_background_threads,
0);
On 2016/11/15 20:31:51, robliao wrote:
> Hitting this DCHECK and the one below seems like it would be pretty bad as no
> workers would be allocated to create tasks.  Maybe we should CHECK instead?

create tasks -> run tasks

[gab, 2016-11-15 20:42:47]

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
content/renderer/render_process_impl.cc:101: DCHECK_GT(max_background_threads,
0);
On 2016/11/15 20:32:36, robliao wrote:
> On 2016/11/15 20:31:51, robliao wrote:
> > Hitting this DCHECK and the one below seems like it would be pretty bad as
no
> > workers would be allocated to create tasks.  Maybe we should CHECK instead?
> 
> create tasks -> run tasks

No need to CHECK IMO, DCHECK documents runtime invariants. CHECK is meant to
verify bad input that could cause security issues.

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[robliao, 2016-11-15 20:49:09]

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
content/renderer/render_process_impl.cc:101: DCHECK_GT(max_background_threads,
0);
On 2016/11/15 20:42:47, gab wrote:
> On 2016/11/15 20:32:36, robliao wrote:
> > On 2016/11/15 20:31:51, robliao wrote:
> > > Hitting this DCHECK and the one below seems like it would be pretty bad as
> no
> > > workers would be allocated to create tasks.  Maybe we should CHECK
instead?
> > 
> > create tasks -> run tasks
> 
> No need to CHECK IMO, DCHECK documents runtime invariants. CHECK is meant to
> verify bad input that could cause security issues.
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

This would be pretty fatal to the process. We already CHECK that
SchedulerWorkerPoolImpl::Initialize provides at least one worker thread because
we won't make progress otherwise. Bringing that CHECK here would cover the input
case (and we wouldn't have to disambiguate between the OOM case and bad inputs).

[gab, 2016-11-15 22:04:30]

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
content/renderer/render_process_impl.cc:101: DCHECK_GT(max_background_threads,
0);
On 2016/11/15 20:49:08, robliao wrote:
> On 2016/11/15 20:42:47, gab wrote:
> > On 2016/11/15 20:32:36, robliao wrote:
> > > On 2016/11/15 20:31:51, robliao wrote:
> > > > Hitting this DCHECK and the one below seems like it would be pretty bad
as
> > no
> > > > workers would be allocated to create tasks.  Maybe we should CHECK
> instead?
> > > 
> > > create tasks -> run tasks
> > 
> > No need to CHECK IMO, DCHECK documents runtime invariants. CHECK is meant to
> > verify bad input that could cause security issues.
> > 
> >
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> 
> This would be pretty fatal to the process. We already CHECK that
> SchedulerWorkerPoolImpl::Initialize provides at least one worker thread
because
> we won't make progress otherwise. Bringing that CHECK here would cover the
input
> case (and we wouldn't have to disambiguate between the OOM case and bad
inputs).

But the CHECK in SchedulerWorkerPoolImpl::Initialize is different IMO (i.e. it's
CHECKing versus the result of invoking OS APIs to create threads which we can't
do without)

A CHECK here is merely verifying input we own (albeit sometimes from server but
don't think we should bother with that).

[fdoray, 2016-11-16 16:40:52]

All done.

Working on the ChromeContentRendererClient CL and will add a link from this CL
once it's ready.

https://codereview.chromium.org/2491823005/diff/100001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/2491823005/diff/100001/content/public/rendere...
content/public/renderer/content_renderer_client.h:142: // background pool and
foreground pool, repectively. |background_reclaim_time|
On 2016/11/15 20:31:51, robliao wrote:
> s/repectively/respectively/ and below.

Done.

https://codereview.chromium.org/2491823005/diff/100001/content/public/rendere...
content/public/renderer/content_renderer_client.h:145: // repectively.
|redirect_sequenced_worker_pool| indicates whether tasks
On 2016/11/15 20:25:05, gab wrote:
> respectively

Done.

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
content/renderer/render_process_impl.cc:74: return traits.priority() ==
base::TaskPriority::BACKGROUND
On 2016/11/15 20:25:05, gab wrote:
> DCHECK(!traits.with_file_io());

Done.

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
content/renderer/render_process_impl.cc:101: DCHECK_GT(max_background_threads,
0);
On 2016/11/15 20:49:08, robliao wrote:
> On 2016/11/15 20:42:47, gab wrote:
> > On 2016/11/15 20:32:36, robliao wrote:
> > > On 2016/11/15 20:31:51, robliao wrote:
> > > > Hitting this DCHECK and the one below seems like it would be pretty bad
as
> > no
> > > > workers would be allocated to create tasks.  Maybe we should CHECK
> instead?
> > > 
> > > create tasks -> run tasks
> > 
> > No need to CHECK IMO, DCHECK documents runtime invariants. CHECK is meant to
> > verify bad input that could cause security issues.
> > 
> >
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> 
> This would be pretty fatal to the process. We already CHECK that
> SchedulerWorkerPoolImpl::Initialize provides at least one worker thread
because
> we won't make progress otherwise. Bringing that CHECK here would cover the
input
> case (and we wouldn't have to disambiguate between the OOM case and bad
inputs).

These DCHECKs will only fail if GetTaskSchedulerInitializationArguments() isn't
properly implemented. Such a problem is easy to catch on developer machines and
there is little value to having CHECKs for it in production code.

The CHECKs in TaskSchedulerImpl::Initialize() may fail even if there is no error
in our code (e.g. CreateThread fails). For our users, a process that is
terminated is better than having a process that can't run tasks.

Since the style guide says that we may only use CHECK to prevent security
vulnerabilities or to temporarily to catch errors in the wild, we might have to
find an alternative way to exit the process when there are errors in
TaskSchedulerImpl::Initialize().

[fdoray, 2016-11-16 16:41:49]

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

[commit-bot: I haz the power, 2016-11-16 16:42:43]

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, 2016-11-16 16:53:42]

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

[commit-bot: I haz the power, 2016-11-16 16:53:42]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2016-11-16 16:58:06]

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
content/renderer/render_process_impl.cc:101: DCHECK_GT(max_background_threads,
0);
On 2016/11/16 16:40:52, fdoray wrote:
> On 2016/11/15 20:49:08, robliao wrote:
> > On 2016/11/15 20:42:47, gab wrote:
> > > On 2016/11/15 20:32:36, robliao wrote:
> > > > On 2016/11/15 20:31:51, robliao wrote:
> > > > > Hitting this DCHECK and the one below seems like it would be pretty
bad
> as
> > > no
> > > > > workers would be allocated to create tasks.  Maybe we should CHECK
> > instead?
> > > > 
> > > > create tasks -> run tasks
> > > 
> > > No need to CHECK IMO, DCHECK documents runtime invariants. CHECK is meant
to
> > > verify bad input that could cause security issues.
> > > 
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> > 
> > This would be pretty fatal to the process. We already CHECK that
> > SchedulerWorkerPoolImpl::Initialize provides at least one worker thread
> because
> > we won't make progress otherwise. Bringing that CHECK here would cover the
> input
> > case (and we wouldn't have to disambiguate between the OOM case and bad
> inputs).
> 
> These DCHECKs will only fail if GetTaskSchedulerInitializationArguments()
isn't
> properly implemented. Such a problem is easy to catch on developer machines
and
> there is little value to having CHECKs for it in production code.

Agreed.

> 
> The CHECKs in TaskSchedulerImpl::Initialize() may fail even if there is no
error
> in our code (e.g. CreateThread fails). For our users, a process that is
> terminated is better than having a process that can't run tasks.
> 
> Since the style guide says that we may only use CHECK to prevent security
> vulnerabilities or to temporarily to catch errors in the wild, we might have
to
> find an alternative way to exit the process when there are errors in
> TaskSchedulerImpl::Initialize().

Meh, I'm fine with the current use of CHECK for hard exit on system failure to
create threads.

[robliao, 2016-11-16 17:00:18]

On 2016/11/16 16:58:06, gab wrote:
>
https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
> File content/renderer/render_process_impl.cc (right):
> 
>
https://codereview.chromium.org/2491823005/diff/100001/content/renderer/rende...
> content/renderer/render_process_impl.cc:101: DCHECK_GT(max_background_threads,
> 0);
> On 2016/11/16 16:40:52, fdoray wrote:
> > On 2016/11/15 20:49:08, robliao wrote:
> > > On 2016/11/15 20:42:47, gab wrote:
> > > > On 2016/11/15 20:32:36, robliao wrote:
> > > > > On 2016/11/15 20:31:51, robliao wrote:
> > > > > > Hitting this DCHECK and the one below seems like it would be pretty
> bad
> > as
> > > > no
> > > > > > workers would be allocated to create tasks.  Maybe we should CHECK
> > > instead?
> > > > > 
> > > > > create tasks -> run tasks
> > > > 
> > > > No need to CHECK IMO, DCHECK documents runtime invariants. CHECK is
meant
> to
> > > > verify bad input that could cause security issues.
> > > > 
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> > > 
> > > This would be pretty fatal to the process. We already CHECK that
> > > SchedulerWorkerPoolImpl::Initialize provides at least one worker thread
> > because
> > > we won't make progress otherwise. Bringing that CHECK here would cover the
> > input
> > > case (and we wouldn't have to disambiguate between the OOM case and bad
> > inputs).
> > 
> > These DCHECKs will only fail if GetTaskSchedulerInitializationArguments()
> isn't
> > properly implemented. Such a problem is easy to catch on developer machines
> and
> > there is little value to having CHECKs for it in production code.
> 
> Agreed.
> 
> > 
> > The CHECKs in TaskSchedulerImpl::Initialize() may fail even if there is no
> error
> > in our code (e.g. CreateThread fails). For our users, a process that is
> > terminated is better than having a process that can't run tasks.
> > 
> > Since the style guide says that we may only use CHECK to prevent security
> > vulnerabilities or to temporarily to catch errors in the wild, we might have
> to
> > find an alternative way to exit the process when there are errors in
> > TaskSchedulerImpl::Initialize().
> 
> Meh, I'm fine with the current use of CHECK for hard exit on system failure to
> create threads.

Agreed on that. My reading on the CHECK is you'd want to fire them at the
earliest point when you know you're going to cause a fatal error. Similar to how
use use DCHECKs for pointers, it identifies at the earliest point in time where
things went wrong.

If we CHECK during initialize, we have to disambiguate between a thread creation
failure and a bad input failure.

[robliao, 2016-11-16 17:02:09]

lgtm in the meantime.

[fdoray, 2016-11-16 17:30:50]

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

[commit-bot: I haz the power, 2016-11-16 17:32:16]

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, 2016-11-16 17:56:12]

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

[commit-bot: I haz the power, 2016-11-16 17:56:13]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[fdoray, 2016-11-18 14:45:59]

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

[commit-bot: I haz the power, 2016-11-18 14:46:19]

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, 2016-11-18 14:56:59]

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

[commit-bot: I haz the power, 2016-11-18 14:57:00]

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

[fdoray, 2016-11-18 15:39:27]

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

[commit-bot: I haz the power, 2016-11-18 15:39:50]

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, 2016-11-18 15:51:43]

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

[commit-bot: I haz the power, 2016-11-18 15:51:44]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[fdoray, 2016-11-18 17:01:31]

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

[commit-bot: I haz the power, 2016-11-18 17:02:44]

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

[fdoray, 2016-11-18 17:03:40]

The CQ bit was unchecked by fdoray@chromium.org

[fdoray, 2016-11-18 17:03:57]

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

[fdoray, 2016-11-18 17:04:18]

PTAnL - GetTaskSchedulerInitializationArguments() now returns an
std::vector<base::SchedulerWorkerPoolParams> and a Callback.

[commit-bot: I haz the power, 2016-11-18 17:05:06]

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, 2016-11-18 17:48:13]

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

[commit-bot: I haz the power, 2016-11-18 17:48:14]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2016-11-18 18:32:00]

https://codereview.chromium.org/2491823005/diff/240001/content/public/rendere...
File content/public/renderer/content_renderer_client.cc (right):

https://codereview.chromium.org/2491823005/diff/240001/content/public/rendere...
content/public/renderer/content_renderer_client.cc:25: FOREGROUND_WORKER_POOL,
Not used anywhere but in GetTaskSchedulerWorkerPoolIndexForTraits()?

I think you had DCHECK_EQ comparing size before insertion with these before, I
guess that'd be one way to highlight where they come from?

Or we could just use 0/1 directly IMO if everything is inlined below and it's
clear.

https://codereview.chromium.org/2491823005/diff/240001/content/public/rendere...
content/public/renderer/content_renderer_client.cc:35: }
I'd prefer for this to be inlined in method's scope.

https://codereview.chromium.org/2491823005/diff/240001/content/public/rendere...
content/public/renderer/content_renderer_client.cc:92: constexpr int
kForegroundThreadsOffset = 0;
These constants are probably too high for the renderers (unlike the browser
there's more than one instance of them).

"8" as a magic number was initially based on 3 BlockingPool + 5 CachePool being
the main consumers in browser land.

Thoughts on what magic numbers make sense for the renderers?

https://codereview.chromium.org/2491823005/diff/240001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/240001/content/renderer/rende...
content/renderer/render_process_impl.cc:80: base::Callback<size_t(const
base::TaskTraits& traits)>
Feels like this type should be defined somewhere.

using SchedulerWorkerPoolSelector = Callback<size_t(const TaskTraits& traits)>;

in task_scheduler.h?

https://codereview.chromium.org/2491823005/diff/240001/content/renderer/rende...
content/renderer/render_process_impl.cc:96:
base::SequencedWorkerPool::RedirectToTaskSchedulerForProcess();
else EnableForProcess?

(won't there be a clash with your other CL if redirect/enable decision isn't in
same place? or is this just because this one doesn't currently depend on the
other?)

[fdoray, 2016-12-08 20:34:38]

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

[commit-bot: I haz the power, 2016-12-08 20:35:32]

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, 2016-12-08 20:52:56]

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

[commit-bot: I haz the power, 2016-12-08 20:52:57]

Dry run: Try jobs failed on following builders:
  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, 2016-12-09 18:03:07]

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

[commit-bot: I haz the power, 2016-12-09 18:04:00]

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

[fdoray, 2016-12-09 18:04:36]

Description was changed from

==========
Initialize TaskScheduler in renderers.

This CL initializes TaskScheduler in renderers using
arguments provided by the ContentRendererClient. By
default, no tasks are redirected to TaskScheduler.

BUG=664996
==========

to

==========
Initialize TaskScheduler in renderers.

This CL initializes TaskScheduler in renderers using
default arguments. ContentRenderClient may provide
different arguments.

BUG=664996
==========

[fdoray, 2016-12-09 18:06:02]

PTAnL - I rewrote everything to mimic TaskScheduler initialization in the
browser process. I'm working on a separate CL to allow
ChromeContentRendererClient to provide its own initialization arguments.

[commit-bot: I haz the power, 2016-12-09 18:28:28]

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

[commit-bot: I haz the power, 2016-12-09 18:28:29]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[gab, 2016-12-09 20:20:56]

https://codereview.chromium.org/2491823005/diff/300001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/300001/content/renderer/rende...
content/renderer/render_process_impl.cc:116:
base::RecommendedMaxNumberOfThreadsInPool(8, 32, 0.3, 0),
Don't think we want nearly that many threads. Let's discuss offline.

[fdoray, 2016-12-09 21:57:14]

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

[fdoray, 2016-12-09 21:57:33]

PTAnL

[commit-bot: I haz the power, 2016-12-09 21:57:58]

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

[gab, 2016-12-09 23:19:43]

lgtm

https://codereview.chromium.org/2491823005/diff/340001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/340001/content/renderer/rende...
content/renderer/render_process_impl.cc:87:
base::RecommendedMaxNumberOfThreadsInPool(1, 2, 1, 0),
Thought we'd said 2-4?

[commit-bot: I haz the power, 2016-12-10 00:31:33]

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

[commit-bot: I haz the power, 2016-12-10 00:31:34]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-12-12 16:16:21]

lgtm

[fdoray, 2016-12-12 16:20:08]

https://codereview.chromium.org/2491823005/diff/340001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/340001/content/renderer/rende...
content/renderer/render_process_impl.cc:87:
base::RecommendedMaxNumberOfThreadsInPool(1, 2, 1, 0),
On 2016/12/09 23:19:42, gab wrote:
> Thought we'd said 2-4?

Done.

[fdoray, 2016-12-12 20:52:15]

Description was changed from

==========
Initialize TaskScheduler in renderers.

This CL initializes TaskScheduler in renderers using
default arguments. ContentRenderClient may provide
different arguments.

BUG=664996
==========

to

==========
Initialize TaskScheduler in renderers.

This CL initializes TaskScheduler in renderers using
default arguments. ContentRenderClient may provide
different arguments.

Initially, no threads will be created by TaskScheduler
in renderers because no tasks are posted to it and
pools are initialized with StandbyThreadPolicy::LAZY.

In a separate, we will start running v8 tasks in
TaskScheduler rather than WorkerPool
(https://cs.chromium.org/chromium/src/gin/v8_platform.cc?sq=package:chromium&l...).

BUG=664996
==========

[fdoray, 2016-12-12 20:52:56]

fdoray@chromium.org changed reviewers:
+ jam@chromium.org

[fdoray, 2016-12-12 20:52:57]

jam@: PTAL

[jam, 2016-12-13 01:28:22]

lgtm

https://codereview.chromium.org/2491823005/diff/360001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/360001/content/renderer/rende...
content/renderer/render_process_impl.cc:80: StandbyThreadPolicy::LAZY, 1,
all the numbers here and below are magic to someone not familiar with it. please
move them to constants

https://codereview.chromium.org/2491823005/diff/360001/content/renderer/rende...
content/renderer/render_process_impl.cc:108: bool IsSingleProcess() {
nit: we don't generally use helper methods for checking one command line flag

[fdoray, 2016-12-13 16:19:52]

https://codereview.chromium.org/2491823005/diff/360001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2491823005/diff/360001/content/renderer/rende...
content/renderer/render_process_impl.cc:80: StandbyThreadPolicy::LAZY, 1,
On 2016/12/13 01:28:22, jam wrote:
> all the numbers here and below are magic to someone not familiar with it.
please
> move them to constants 

Done.

https://codereview.chromium.org/2491823005/diff/360001/content/renderer/rende...
content/renderer/render_process_impl.cc:108: bool IsSingleProcess() {
On 2016/12/13 01:28:22, jam wrote:
> nit: we don't generally use helper methods for checking one command line flag

Done.

[fdoray, 2016-12-13 16:19:59]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-12-13 16:19:59]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
robliao@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2491823005/#ps380001
(title: "CR jam #76")

[commit-bot: I haz the power, 2016-12-13 16:20: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, 2016-12-13 17:18:28]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1481645999047150,
"parent_rev": "f12925ef31acf576805b97c6caf8bbc002864652", "commit_rev":
"6ff58fd004a739e9c0c95fad2c73131a7587ed08"}

[commit-bot: I haz the power, 2016-12-13 17:19:34]

Description was changed from

==========
Initialize TaskScheduler in renderers.

This CL initializes TaskScheduler in renderers using
default arguments. ContentRenderClient may provide
different arguments.

Initially, no threads will be created by TaskScheduler
in renderers because no tasks are posted to it and
pools are initialized with StandbyThreadPolicy::LAZY.

In a separate, we will start running v8 tasks in
TaskScheduler rather than WorkerPool
(https://cs.chromium.org/chromium/src/gin/v8_platform.cc?sq=package:chromium&l...).

BUG=664996
==========

to

==========
Initialize TaskScheduler in renderers.

This CL initializes TaskScheduler in renderers using
default arguments. ContentRenderClient may provide
different arguments.

Initially, no threads will be created by TaskScheduler
in renderers because no tasks are posted to it and
pools are initialized with StandbyThreadPolicy::LAZY.

In a separate, we will start running v8 tasks in
TaskScheduler rather than WorkerPool
(https://cs.chromium.org/chromium/src/gin/v8_platform.cc?sq=package:chromium&l...).

BUG=664996

Review-Url: https://codereview.chromium.org/2491823005
==========

[commit-bot: I haz the power, 2016-12-13 17:19:38]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-12-13 17:23:44]

Description was changed from

==========
Initialize TaskScheduler in renderers.

This CL initializes TaskScheduler in renderers using
default arguments. ContentRenderClient may provide
different arguments.

Initially, no threads will be created by TaskScheduler
in renderers because no tasks are posted to it and
pools are initialized with StandbyThreadPolicy::LAZY.

In a separate, we will start running v8 tasks in
TaskScheduler rather than WorkerPool
(https://cs.chromium.org/chromium/src/gin/v8_platform.cc?sq=package:chromium&l...).

BUG=664996

Review-Url: https://codereview.chromium.org/2491823005
==========

to

==========
Initialize TaskScheduler in renderers.

This CL initializes TaskScheduler in renderers using
default arguments. ContentRenderClient may provide
different arguments.

Initially, no threads will be created by TaskScheduler
in renderers because no tasks are posted to it and
pools are initialized with StandbyThreadPolicy::LAZY.

In a separate, we will start running v8 tasks in
TaskScheduler rather than WorkerPool
(https://cs.chromium.org/chromium/src/gin/v8_platform.cc?sq=package:chromium&l...).

BUG=664996

Committed: https://crrev.com/d2233a758df698c669341ef2e49b1ad2b23ce8da
Cr-Commit-Position: refs/heads/master@{#438202}
==========

[commit-bot: I haz the power, 2016-12-13 17:23:46]

Patchset 20 (id:??) landed as
https://crrev.com/d2233a758df698c669341ef2e49b1ad2b23ce8da
Cr-Commit-Position: refs/heads/master@{#438202}