scheduler: Add an unthrottled per-frame task runner

scheduler: Add an unthrottled per-frame task runner

Add an unthrottled task runner which will be used for executing internal
browser tasks which should never be throttled. In general only tasks
whose performance characteristics are known should be posted to this
task runner; for example user JavaScript should not be allowed.

BUG=624699
Committed: https://crrev.com/b25671fdd0a363d32f328acdefdf7e6ad9eef218
Cr-Commit-Position: refs/heads/master@{#403670}

[Sami, 2016-07-01 10:59:46]

Description was changed from

==========
scheduler: Add an unthrottled per-frame task runner

Add an unthrottled task runner which will be used for executing internal
browser tasks which should never be throttled. In general only tasks
whose performance characteristics are known should be posted to this
task runner; for example user JavaScript should not be allowed.

BUG=624699
==========

to

==========
scheduler: Add an unthrottled per-frame task runner

Add an unthrottled task runner which will be used for executing internal
browser tasks which should never be throttled. In general only tasks
whose performance characteristics are known should be posted to this
task runner; for example user JavaScript should not be allowed.

BUG=624699
==========

[Sami, 2016-07-01 10:59:46]

skyostil@chromium.org changed reviewers:
+ alexclarke@chromium.org, haraken@chromium.org

[Sami, 2016-07-01 11:01:20]

PTAL.

By the way, one thing to note about this task runner is that it won't get
suspended the way timers do, e.g., during nested message loops. If we do want
that behavior, we can adjust the policy in the renderer scheduler.

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

lgtm

https://codereview.chromium.org/2118783002/diff/1/components/scheduler/render...
File components/scheduler/renderer/renderer_scheduler.h (right):

https://codereview.chromium.org/2118783002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler.h:71: // Returns a task runner
for tasks which should never get throttled.
Not necessarily for this patch, but I wonder if we should consider adding a
builder interface here, especially since we're thinking of supporting sub-task
queues and worklets (if those get traction).

[haraken, 2016-07-01 13:04:01]

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

[haraken, 2016-07-01 13:04:01]

Thanks for working on this! WebKit/ LGTM.

[Sami, 2016-07-01 13:11:43]

https://codereview.chromium.org/2118783002/diff/1/components/scheduler/render...
File components/scheduler/renderer/renderer_scheduler.h (right):

https://codereview.chromium.org/2118783002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler.h:71: // Returns a task runner
for tasks which should never get throttled.
On 2016/07/01 11:30:05, alex clarke wrote:
> Not necessarily for this patch, but I wonder if we should consider adding a
> builder interface here, especially since we're thinking of supporting sub-task
> queues and worklets (if those get traction).

That's a fine idea -- let's keep it in mind.

[Sami, 2016-07-01 17:53:51]

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

[commit-bot: I haz the power, 2016-07-01 17:54:10]

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-07-01 18:42:08]

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

[commit-bot: I haz the power, 2016-07-01 18:42:09]

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_...)

[dcheng, 2016-07-04 06:52:35]

//third_party/WebKit/public LGTM with a followup question

https://codereview.chromium.org/2118783002/diff/1/components/scheduler/render...
File components/scheduler/renderer/web_frame_scheduler_impl.cc (right):

https://codereview.chromium.org/2118783002/diff/1/components/scheduler/render...
components/scheduler/renderer/web_frame_scheduler_impl.cc:40: if
(unthrottled_task_queue_.get()) {
Nit: no .get() here

https://codereview.chromium.org/2118783002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebFrameScheduler.h (right):

https://codereview.chromium.org/2118783002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebFrameScheduler.h:37: // WebFrameScheduler
owns the returned WebTaskRunner.
Will we have some sort of documentation for what tasks go in which queue? For
example, this CL changes one line in FrameView to use the unthrottled task
runner, but I'm not sure why =)

[alex clarke (OOO till 29th), 2016-07-04 09:02:19]

https://codereview.chromium.org/2118783002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebFrameScheduler.h (right):

https://codereview.chromium.org/2118783002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebFrameScheduler.h:37: // WebFrameScheduler
owns the returned WebTaskRunner.
On 2016/07/04 06:52:35, dcheng wrote:
> Will we have some sort of documentation for what tasks go in which queue? For
> example, this CL changes one line in FrameView to use the unthrottled task
> runner, but I'm not sure why =)

Scheduling decisions (i.e. should a frame be throttled) are based on the
notification delivered by ThrottlingObserverNotificationFactory.  This being
posted on a potentially throttled queue is undesirable since it could take up to
a second for task queues associated with the frame to become unthrottled when it
becomes visible.

Posting on the unthrottled queue should make it happen almost immediately.

[Sami, 2016-07-04 09:28:43]

Thanks for the review.

https://codereview.chromium.org/2118783002/diff/1/components/scheduler/render...
File components/scheduler/renderer/web_frame_scheduler_impl.cc (right):

https://codereview.chromium.org/2118783002/diff/1/components/scheduler/render...
components/scheduler/renderer/web_frame_scheduler_impl.cc:40: if
(unthrottled_task_queue_.get()) {
On 2016/07/04 06:52:35, dcheng wrote:
> Nit: no .get() here

Done.

https://codereview.chromium.org/2118783002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebFrameScheduler.h (right):

https://codereview.chromium.org/2118783002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebFrameScheduler.h:37: // WebFrameScheduler
owns the returned WebTaskRunner.
On 2016/07/04 09:02:19, alex clarke wrote:
> On 2016/07/04 06:52:35, dcheng wrote:
> > Will we have some sort of documentation for what tasks go in which queue?
For
> > example, this CL changes one line in FrameView to use the unthrottled task
> > runner, but I'm not sure why =)
> 
> Scheduling decisions (i.e. should a frame be throttled) are based on the
> notification delivered by ThrottlingObserverNotificationFactory.  This being
> posted on a potentially throttled queue is undesirable since it could take up
to
> a second for task queues associated with the frame to become unthrottled when
it
> becomes visible.
> 
> Posting on the unthrottled queue should make it happen almost immediately.

I've added a bit more documentation here to hopefully make the intended usage
clearer.

[Sami, 2016-07-04 09:28:54]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2016-07-04 09:28:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, dcheng@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2118783002/#ps20001
(title: "Nits")

[commit-bot: I haz the power, 2016-07-04 09:29:09]

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-07-04 10:19:26]

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

[commit-bot: I haz the power, 2016-07-04 10:19:27]

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_...)

[Sami, 2016-07-04 11:16:45]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2016-07-04 11:16:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, dcheng@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2118783002/#ps40001
(title: "Add implementation in EmptyClients")

[commit-bot: I haz the power, 2016-07-04 11:17:00]

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-07-04 12:29:57]

Description was changed from

==========
scheduler: Add an unthrottled per-frame task runner

Add an unthrottled task runner which will be used for executing internal
browser tasks which should never be throttled. In general only tasks
whose performance characteristics are known should be posted to this
task runner; for example user JavaScript should not be allowed.

BUG=624699
==========

to

==========
scheduler: Add an unthrottled per-frame task runner

Add an unthrottled task runner which will be used for executing internal
browser tasks which should never be throttled. In general only tasks
whose performance characteristics are known should be posted to this
task runner; for example user JavaScript should not be allowed.

BUG=624699
==========

[commit-bot: I haz the power, 2016-07-04 12:29:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-04 12:31:36]

Description was changed from

==========
scheduler: Add an unthrottled per-frame task runner

Add an unthrottled task runner which will be used for executing internal
browser tasks which should never be throttled. In general only tasks
whose performance characteristics are known should be posted to this
task runner; for example user JavaScript should not be allowed.

BUG=624699
==========

to

==========
scheduler: Add an unthrottled per-frame task runner

Add an unthrottled task runner which will be used for executing internal
browser tasks which should never be throttled. In general only tasks
whose performance characteristics are known should be posted to this
task runner; for example user JavaScript should not be allowed.

BUG=624699

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

[commit-bot: I haz the power, 2016-07-04 12:31:37]

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