Suspend frame schedulers on a page suspension

Suspend frame schedulers on a page suspension

ExecutionContext has its own task suspension through MainThreadTaskRunner.
However, the suspension can be coarser, since it's used only when the page
is suspended. Also, the custom suspension / resumption makes it hard to
use finer grained TaskType.
This CL moves the task suspension from ExecutionContext to WebFrameScheduler.

Committed: https://crrev.com/c4c7589017c64f0881dab16fdf5d7594099f1d7e
Cr-Commit-Position: refs/heads/master@{#435588}

[tzik, 2016-11-24 11:42:53]

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

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

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-24 13:44:24]

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

[commit-bot: I haz the power, 2016-11-24 13:44:25]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[tzik, 2016-11-24 17:24:54]

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

[commit-bot: I haz the power, 2016-11-24 17:25:34]

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-24 18:35:08]

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

[commit-bot: I haz the power, 2016-11-24 18:35:09]

Dry run: This issue passed the CQ dry run.

[tzik, 2016-11-25 05:26:56]

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

[commit-bot: I haz the power, 2016-11-25 05:27:09]

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

[tzik, 2016-11-25 05:35:16]

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

[commit-bot: I haz the power, 2016-11-25 05:35:21]

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

[tzik, 2016-11-25 06:32:08]

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

[commit-bot: I haz the power, 2016-11-25 06:32:20]

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

[tzik, 2016-11-25 06:37:37]

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

[commit-bot: I haz the power, 2016-11-25 06:37:52]

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

[tzik, 2016-11-25 06:46:42]

Description was changed from

==========
Suspend frame schedulers on a page suspension

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

to

==========
Suspend frame schedulers on a page suspension

ExecutionContext has its own task suspension through MainThreadTaskRunner.
However, the suspension can be coarser, since it's used only when the page
is suspended. Also, the custom suspension / resumption makes it hard to
use finer grained TaskType.
This CL moves the task suspension from ExecutionContext to WebFrameScheduler.
==========

[tzik, 2016-11-25 06:47:10]

tzik@chromium.org changed reviewers:
+ haraken@chromium.org

[tzik, 2016-11-25 06:47:45]

tzik@chromium.org changed reviewers:
+ alexclarke@chromium.org

[tzik, 2016-11-25 06:52:13]

PTAL

[commit-bot: I haz the power, 2016-11-25 08:12:12]

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

[commit-bot: I haz the power, 2016-11-25 08:12:13]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2016-11-25 09:50:39]

https://codereview.chromium.org/2527143002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc
(right):

https://codereview.chromium.org/2527143002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:178:
renderer_scheduler_->task_queue_throttler()->SetQueueEnabled(
What you're trying to do here makes sense. Unfortunately this is going to get
overwritten by RendererSchedulerImpl::ApplyTaskQueuePolicy. We should be able to
support this but we're going to need to do something a bit more complex.  I have
an idea but I want to discuss it with Sami when he gets in.

[haraken, 2016-11-25 10:20:19]

This CL makes sense to me.

Before this CL, we were stopping only the main thread task runner, but that
doesn't make much sense because the main thread task runner contains only a part
of all tasks. It would make more sense to stop the per-frame scheduler (i.e.,
what this CL is doing).

[Sami, 2016-11-25 11:32:27]

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

[Sami, 2016-11-25 11:32:28]

Seems like a good idea!

https://codereview.chromium.org/2527143002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc
(right):

https://codereview.chromium.org/2527143002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:178:
renderer_scheduler_->task_queue_throttler()->SetQueueEnabled(
On 2016/11/25 09:50:39, alex clarke wrote:
> What you're trying to do here makes sense. Unfortunately this is going to get
> overwritten by RendererSchedulerImpl::ApplyTaskQueuePolicy. We should be able
to
> support this but we're going to need to do something a bit more complex.  I
have
> an idea but I want to discuss it with Sami when he gets in.

We chatted a bit with Alex and are feeling that we should probably add a little
more structure around enabling and disabling task queues. We came up with this
straw man:

- each TaskQueue gets a CreateEnableVoter() method (name to-be-bikeshedded)
- The voter has two methods: Enable() and Disable()
- If all voters agree to enable, then the task queue remains enabled; otherwise
it is disabled
- When a voter is destroyed, the corresponding vote is removed from the pool.

Essentially this is a refcounted system with the added safety that  when a voter
goes away, there's no risk of unbalancing the count.

How does this sound? We can take a stab at implementing it if seems reasonable.
(We'll need to do some fence-related rework first.)

https://codereview.chromium.org/2527143002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:181:
timer_task_queue_.get(), !frame_suspended);
Note that these queues get created on-demand so you should check that they exist
first.

[alex clarke (OOO till 29th), 2016-11-29 18:09:42]

https://codereview.chromium.org/2527143002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc
(right):

https://codereview.chromium.org/2527143002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:178:
renderer_scheduler_->task_queue_throttler()->SetQueueEnabled(
To support this, I've landed a patch adding TaskQueue::QueueEnabledVoter.

To use it you'd do something like this:

In the header:

unique_ptr<TaskQueue::QueueEnabledVoter> loading_task_queue_enabled_voter_;

unique_ptr<TaskQueue::QueueEnabledVoter> timer_task_queue_enabled_voter_;

In WebFrameSchedulerImpl::loadingTaskRunner after >NewLoadingTaskRunner:
loading_task_queue_enabled_voter_ =
loading_task_queue_->CreateQueueEnabledVoter();

In WebFrameSchedulerImpl::timerTaskRunner after >NewLoadingTaskRunneR:
timer_task_queue_enabled_voter_ = timer_task_queue_->CreateQueueEnabledVoter();

In  WebFrameSchedulerImpl::setSuspended:
loading_task_queue_enabled_voter_->SetQueueEnabled(!frame_suspended);
timer_task_queue_enabled_voter_->SetQueueEnabled(!frame_suspended);

[tzik, 2016-11-30 07:22:57]

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

[commit-bot: I haz the power, 2016-11-30 07:23:38]

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

[tzik, 2016-11-30 08:34:36]

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

[commit-bot: I haz the power, 2016-11-30 08:35:24]

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

[tzik, 2016-11-30 08:44:49]

https://codereview.chromium.org/2527143002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc
(right):

https://codereview.chromium.org/2527143002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:178:
renderer_scheduler_->task_queue_throttler()->SetQueueEnabled(
On 2016/11/29 18:09:42, alex clarke wrote:
> To support this, I've landed a patch adding TaskQueue::QueueEnabledVoter.
> 
> To use it you'd do something like this:
> 
> In the header:
> 
> unique_ptr<TaskQueue::QueueEnabledVoter> loading_task_queue_enabled_voter_;
> 
> unique_ptr<TaskQueue::QueueEnabledVoter> timer_task_queue_enabled_voter_;
> 
> In WebFrameSchedulerImpl::loadingTaskRunner after >NewLoadingTaskRunner:
> loading_task_queue_enabled_voter_ =
> loading_task_queue_->CreateQueueEnabledVoter();
> 
> In WebFrameSchedulerImpl::timerTaskRunner after >NewLoadingTaskRunneR:
> timer_task_queue_enabled_voter_ =
timer_task_queue_->CreateQueueEnabledVoter();
> 
> In  WebFrameSchedulerImpl::setSuspended:
> loading_task_queue_enabled_voter_->SetQueueEnabled(!frame_suspended);
> timer_task_queue_enabled_voter_->SetQueueEnabled(!frame_suspended);

Great!
Updated the CL to use QueueEnabledVoter.

https://codereview.chromium.org/2527143002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:181:
timer_task_queue_.get(), !frame_suspended);
On 2016/11/25 11:32:28, Sami wrote:
> Note that these queues get created on-demand so you should check that they
exist
> first.

Done.

[commit-bot: I haz the power, 2016-11-30 08:47:05]

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

[commit-bot: I haz the power, 2016-11-30 08:47:06]

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

[tzik, 2016-11-30 09:09:42]

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

[commit-bot: I haz the power, 2016-11-30 09:10:00]

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

[alex clarke (OOO till 29th), 2016-11-30 10:29:07]

lgtm

[haraken, 2016-11-30 10:50:57]

LGTM

[commit-bot: I haz the power, 2016-11-30 11:13:01]

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

[commit-bot: I haz the power, 2016-11-30 11:13:02]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-11-30 17:16:19]

lgtm.

https://codereview.chromium.org/2527143002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebFrameScheduler.h (right):

https://codereview.chromium.org/2527143002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebFrameScheduler.h:31: //
unthrottledTaskRunner only on a suspended frame.
nit: this makes it sound like unthrottledTaskRunner only runs when the frame is
suspended. How about:

"Only unthrottledTaskRunner tasks are allowed to run on a suspended frame."

[tzik, 2016-12-01 06:39:02]

https://codereview.chromium.org/2527143002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebFrameScheduler.h (right):

https://codereview.chromium.org/2527143002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebFrameScheduler.h:31: //
unthrottledTaskRunner only on a suspended frame.
On 2016/11/30 17:16:19, Sami wrote:
> nit: this makes it sound like unthrottledTaskRunner only runs when the frame
is
> suspended. How about:
> 
> "Only unthrottledTaskRunner tasks are allowed to run on a suspended frame."

Done.

[tzik, 2016-12-01 06:39:09]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-12-01 06:39:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, skyostil@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2527143002/#ps160001
(title: "mod a comment")

[commit-bot: I haz the power, 2016-12-01 06:39:19]

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-01 09:06:30]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480574349175820,
"parent_rev": "ebc4d8e11148f43ee09aef3876977895491813b2", "commit_rev":
"14e6ff957426b23ba58d78b6f785564cce802c03"}

[commit-bot: I haz the power, 2016-12-01 09:06:57]

Description was changed from

==========
Suspend frame schedulers on a page suspension

ExecutionContext has its own task suspension through MainThreadTaskRunner.
However, the suspension can be coarser, since it's used only when the page
is suspended. Also, the custom suspension / resumption makes it hard to
use finer grained TaskType.
This CL moves the task suspension from ExecutionContext to WebFrameScheduler.
==========

to

==========
Suspend frame schedulers on a page suspension

ExecutionContext has its own task suspension through MainThreadTaskRunner.
However, the suspension can be coarser, since it's used only when the page
is suspended. Also, the custom suspension / resumption makes it hard to
use finer grained TaskType.
This CL moves the task suspension from ExecutionContext to WebFrameScheduler.
==========

[commit-bot: I haz the power, 2016-12-01 09:06:58]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-01 09:08:29]

Description was changed from

==========
Suspend frame schedulers on a page suspension

ExecutionContext has its own task suspension through MainThreadTaskRunner.
However, the suspension can be coarser, since it's used only when the page
is suspended. Also, the custom suspension / resumption makes it hard to
use finer grained TaskType.
This CL moves the task suspension from ExecutionContext to WebFrameScheduler.
==========

to

==========
Suspend frame schedulers on a page suspension

ExecutionContext has its own task suspension through MainThreadTaskRunner.
However, the suspension can be coarser, since it's used only when the page
is suspended. Also, the custom suspension / resumption makes it hard to
use finer grained TaskType.
This CL moves the task suspension from ExecutionContext to WebFrameScheduler.

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

[commit-bot: I haz the power, 2016-12-01 09:08:30]

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