scheduler: Throttle timers in out-of-view frames

scheduler: Throttle timers in out-of-view frames

Throttle timer tasks to 1 Hz in out-of-view or invisible frames. This
reduces the power consumption and performance impact of these tasks,
which are not likely having any visible effect on the page anyway.

BUG=616519
Committed: https://crrev.com/7fec349e59e40dd9beee5f7eafa9e0db5c4ac1bd
Cr-Commit-Position: refs/heads/master@{#415278}

[Sami, 2016-06-28 16:51:29]

Description was changed from

==========
scheduler: Throttle timers in out-of-view frames

Throttle timer tasks to 1 Hz in out-of-view or invisible frames. This
reduces the power consumption and performance impact of these tasks,
which are not likely having any visible effect on the page anyway.

BUG=616519
==========

to

==========
scheduler: Throttle timers in out-of-view frames

Throttle timer tasks to 1 Hz in out-of-view or invisible frames. This
reduces the power consumption and performance impact of these tasks,
which are not likely having any visible effect on the page anyway.

BUG=616519
==========

[Sami, 2016-06-28 16:51:30]

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

[Sami, 2016-06-28 16:51:41]

PTAL.

[alex clarke (OOO till 29th), 2016-06-28 17:11:28]

Plumbing looks fairly sensible.  Would be good to get those test failures fixed
before I look into this in more detail.

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
File components/scheduler/renderer/renderer_web_scheduler_impl.cc (right):

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_web_scheduler_impl.cc:19: const
base::Feature kHiddenTimerThrottlingFeature{
nit: have you run git cl format?

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_web_scheduler_impl.cc:44: bool
allow_hidden_timer_throttling = true;
Wait we're enabling this be default? Is that the right thing to do?

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
File components/scheduler/renderer/web_frame_scheduler_impl.cc (right):

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
components/scheduler/renderer/web_frame_scheduler_impl.cc:136:
parent_web_view_scheduler_->virtual_time_domain()) {
This probably needs a comment explaining that we don't try to throttle a view
that has virtual time enabled.

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
File components/scheduler/renderer/web_frame_scheduler_impl.h (right):

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
components/scheduler/renderer/web_frame_scheduler_impl.h:65: bool
frame_visible_;
I can't help thinking this state would be useful to have in tracing somehow. 
Would you mind putting in a todo?

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
components/scheduler/renderer/web_frame_scheduler_impl.h:68: bool
allow_hidden_timer_throttling_;
const

https://codereview.chromium.org/2023033003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2023033003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4138: void
FrameView::updateThrottlingStatus()
Does this get called often?

[Sami, 2016-06-28 17:22:46]

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
File components/scheduler/renderer/renderer_web_scheduler_impl.cc (right):

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_web_scheduler_impl.cc:19: const
base::Feature kHiddenTimerThrottlingFeature{
On 2016/06/28 17:11:28, alex clarke wrote:
> nit: have you run git cl format?

Yep, looks like this is what it wants to do with initializers :P

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_web_scheduler_impl.cc:44: bool
allow_hidden_timer_throttling = true;
On 2016/06/28 17:11:28, alex clarke wrote:
> Wait we're enabling this be default? Is that the right thing to do?

Yes, we're planning to use Finch to do a hold-out test, which is why I'm adding
a feature flag. I won't actually land this until I get 3xLGTM from my intent to
ship.

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
File components/scheduler/renderer/web_frame_scheduler_impl.cc (right):

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
components/scheduler/renderer/web_frame_scheduler_impl.cc:136:
parent_web_view_scheduler_->virtual_time_domain()) {
On 2016/06/28 17:11:28, alex clarke wrote:
> This probably needs a comment explaining that we don't try to throttle a view
> that has virtual time enabled.

Good point, done. (At some point we'll need to decide how throttling should
happen in headless mode.)

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
File components/scheduler/renderer/web_frame_scheduler_impl.h (right):

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
components/scheduler/renderer/web_frame_scheduler_impl.h:65: bool
frame_visible_;
On 2016/06/28 17:11:28, alex clarke wrote:
> I can't help thinking this state would be useful to have in tracing somehow. 
> Would you mind putting in a todo?

Yes, I thought about that too. Let's figure out a way to do it in a follow-up.

https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/re...
components/scheduler/renderer/web_frame_scheduler_impl.h:68: bool
allow_hidden_timer_throttling_;
On 2016/06/28 17:11:28, alex clarke wrote:
> const

Done.

https://codereview.chromium.org/2023033003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2023033003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4138: void
FrameView::updateThrottlingStatus()
On 2016/06/28 17:11:28, alex clarke wrote:
> Does this get called often?

Only when a frame enters or leaves the viewport. Movement fully inside or
outside the viewport doesn't trigger this work.

[Sami, 2016-06-28 17:41:18]

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

[Sami, 2016-06-28 17:44:46]

Okay, I think I've found the problem with the tests: frame visibility updates
were getting posted as per-frame timer tasks, so if a frame was throttled it
didn't get the message about being unthrottled until up to a second later.

I've fixed this by introducing a "default" per-frame task runner and using it to
process the frame visibility notifications. Since these tasks are internal Blink
tasks that don't run user JS, I don't think we need to throttle them. +haraken@,
this may become useful for moving more work into per-frame task queues.

Ojan, wanna have a look here too?

[haraken, 2016-06-29 01:00:23]

On 2016/06/28 17:44:46, Sami wrote:
> Okay, I think I've found the problem with the tests: frame visibility updates
> were getting posted as per-frame timer tasks, so if a frame was throttled it
> didn't get the message about being unthrottled until up to a second later.
> 
> I've fixed this by introducing a "default" per-frame task runner and using it
to
> process the frame visibility notifications. Since these tasks are internal
Blink
> tasks that don't run user JS, I don't think we need to throttle them.
+haraken@,
> this may become useful for moving more work into per-frame task queues.
> 
> Ojan, wanna have a look here too?

Yeah, the default task runner makes sense to put other per-frame tasks.

[ojan, 2016-06-29 04:53:00]

I'm a little worried about the naming here. If it's the "default" then people
will use it. In practice, this is actually the nonThrottledTaskRunner or
something like that, right? People should basically never use it. Or am I
misunderstanding?

[esprehn, 2016-06-29 05:06:48]

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

[esprehn, 2016-06-29 05:06:49]

https://codereview.chromium.org/2023033003/diff/100001/components/scheduler/r...
File components/scheduler/renderer/web_frame_scheduler_impl.h (right):

https://codereview.chromium.org/2023033003/diff/100001/components/scheduler/r...
components/scheduler/renderer/web_frame_scheduler_impl.h:71: const bool
allow_hidden_timer_throttling_;
I'm sad we keep plumbing more and more stuff out of blink. The scheduler is
moving in the exact opposite direction of everyone else and the Onion Soup plan.
We really really need to stop doing this and get the scheduler code moved into
blink. This is all technical debt.

[esprehn, 2016-06-29 05:09:32]

Hmm can you explain this patch? This is moving us away from being able to use
IntersectionObserver for this feature, since you're using magic scheduling code
in the viewport intersection system for render throttling, but I thought we
wanted to switch that code to use IntersectionObserver which means all of that
code should go away?

[Sami, 2016-06-29 10:20:12]

> I'm a little worried about the naming here. If it's the "default" then people
> will use it. In practice, this is actually the nonThrottledTaskRunner or
> something like that, right? People should basically never use it. Or am I
> misunderstanding?

Yes, we're not expecting many places to use this. Internally we're calling these
types of tasks "control tasks". Maybe that would fit here too.

On 2016/06/29 05:09:32, esprehn wrote:
> Hmm can you explain this patch? This is moving us away from being able to use
> IntersectionObserver for this feature, since you're using magic scheduling
code
> in the viewport intersection system for render throttling, but I thought we
> wanted to switch that code to use IntersectionObserver which means all of that
> code should go away?

Do you mean using the IntersectionObserverController for driving the frame
throttling logic? I may have missed that thread, but now that you mention it I
think it makes sense. If I understood right, this code won't go away even if we
do that -- it will just get triggered differently.

> I'm sad we keep plumbing more and more stuff out of blink. The scheduler is
> moving in the exact opposite direction of everyone else and the Onion Soup
plan.
> We really really need to stop doing this and get the scheduler code moved into
> blink. This is all technical debt.

We've been holding off on the move to make sure all the prerequisites are in
place. Do you think now is the right time? Specifically:

- Can we depend on components/ and cc/ from platform/scheduler (assuming this is
the right place for the scheduler)?
- Can we use Chromium code style?
- content/ needs to use the scheduler too. Is it okay to expose it?

(I'll go watch your BlinkOn Onion Soup talk now...)

[esprehn, 2016-06-30 03:02:39]

On 2016/06/29 at 10:20:12, skyostil wrote:
> > I'm a little worried about the naming here. If it's the "default" then
people
> > will use it. In practice, this is actually the nonThrottledTaskRunner or
> > something like that, right? People should basically never use it. Or am I
> > misunderstanding?
> 
> Yes, we're not expecting many places to use this. Internally we're calling
these types of tasks "control tasks". Maybe that would fit here too.
> 
> On 2016/06/29 05:09:32, esprehn wrote:
> > Hmm can you explain this patch? This is moving us away from being able to
use
> > IntersectionObserver for this feature, since you're using magic scheduling
code
> > in the viewport intersection system for render throttling, but I thought we
> > wanted to switch that code to use IntersectionObserver which means all of
that
> > code should go away?
> 
> Do you mean using the IntersectionObserverController for driving the frame
throttling logic? I may have missed that thread, but now that you mention it I
think it makes sense. If I understood right, this code won't go away even if we
do that -- it will just get triggered differently.

I mean quite literally we should literally do new
ElementVisibilityObserver(iframe, bind(&toggleThrottling)) for each cross origin
iframe and let the intersection observer machinery handle this. It's posting
regular tasks though, so you need to deal with that.

> 
> > I'm sad we keep plumbing more and more stuff out of blink. The scheduler is
> > moving in the exact opposite direction of everyone else and the Onion Soup
plan.
> > We really really need to stop doing this and get the scheduler code moved
into
> > blink. This is all technical debt.
> 
> We've been holding off on the move to make sure all the prerequisites are in
place. Do you think now is the right time? Specifically:
> 
> - Can we depend on components/ and cc/ from platform/scheduler (assuming this
is the right place for the scheduler)?

You can depend on anything that doesn't depend back on Blink. So you can depend
on a component/ that's fully isolated from the rest of the system. So
components/scheduler should expose a bunch of lego blocks and blink should
assemble them.

> - Can we use Chromium code style?

Depends what you mean. If you're moving code you definitely don't need to
rewrite the style. Please do use WTF, and leverage the power of Oilpan to solve
lifetime issues, but you don't need to reformat all your code. We can add an
exception to the presubmit as needed.

> - content/ needs to use the scheduler too. Is it okay to expose it?
>

That depends, content/ is mostly going away. What needs the scheduler and why?
We probably don't need to expose the full scheduler, instead you can expose some
limited APIs and have the glue between cc/ and blink done inside platform and
core itself.
 
Think of the scheduler more like CoreAnimation and Grand Central Dispatch. Those
systems don't know anything about frames, or cross origin, or urls, or
navigation or dom. Instead you assemble the building blocks into your usage. The
scheduler should be those building blocks of task queues, priorities, ordering,
dependency between queues, etc. Blink should assemble them into a graph that's
useful for us.

[haraken, 2016-07-01 05:31:27]

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

[haraken, 2016-07-01 05:31:27]

FWIW, the per-iframe scheduler work
(https://docs.google.com/document/d/10It1DFRP7H3gev9hQA-7dtTcvcMFhrxuCt3-k21yj...)
needs the unthrottled task runner on WebFrameScheduler. I'm a big fan of the
idea of moving the scheduler to platform/, but if it takes a couple of weeks,
I'd prefer landing this patch to unblock the per-iframe scheduler work.

[haraken, 2016-07-01 05:32:06]

On 2016/07/01 05:31:27, haraken wrote:
> FWIW, the per-iframe scheduler work
>
(https://docs.google.com/document/d/10It1DFRP7H3gev9hQA-7dtTcvcMFhrxuCt3-k21yj...)
> needs the unthrottled task runner on WebFrameScheduler. I'm a big fan of the
> idea of moving the scheduler to platform/, but if it takes a couple of weeks,
> I'd prefer landing this patch to unblock the per-iframe scheduler work.

s/land this patch/land the unthrottled task runner/.

[Sami, 2016-07-01 11:02:38]

On 2016/07/01 05:32:06, haraken wrote:
> On 2016/07/01 05:31:27, haraken wrote:
> > FWIW, the per-iframe scheduler work
> >
>
(https://docs.google.com/document/d/10It1DFRP7H3gev9hQA-7dtTcvcMFhrxuCt3-k21yj...)
> > needs the unthrottled task runner on WebFrameScheduler. I'm a big fan of the
> > idea of moving the scheduler to platform/, but if it takes a couple of
weeks,
> > I'd prefer landing this patch to unblock the per-iframe scheduler work.
> 
> s/land this patch/land the unthrottled task runner/.

Agreed, I've split that out to its own patch here:
https://codereview.chromium.org/2118783002/

I'll investigate what it would take to move the scheduler next.

[Sami, 2016-08-25 13:41:10]

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

[commit-bot: I haz the power, 2016-08-25 13:41:31]

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-08-25 15:27:23]

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

[commit-bot: I haz the power, 2016-08-25 15:27:23]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-08-25 16:29:06]

Okay, I've picked this up again now that the scheduler is in Blink. PTAL.

FYI, I've got another patch in flight to make FrameView use
IntersectionObserver: https://codereview.chromium.org/2272773002/. I don't think
we need to block this one on that though.

[alex clarke (OOO till 29th), 2016-08-25 16:39:24]

lgtm

[alex clarke (OOO till 29th), 2016-08-25 16:39:26]

lgtm

[haraken, 2016-08-26 02:10:52]

LGTM

I'm just curious but what metrics are you going to keep track of to measure the
impact of the offscreen throttling?

[Sami, 2016-08-26 10:02:56]

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

[Sami, 2016-08-26 10:04:36]

On 2016/08/26 02:10:52, haraken wrote:
> LGTM

Thanks! +jochen@, could you check content/ please?

> I'm just curious but what metrics are you going to keep track of to measure
the
> impact of the offscreen throttling?

Thanks! Looks like I forgot to mention that in my intent to ship, but I'm
planning to use the same metrics we did for rAF throttling:

https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/NJIVCmBAwkw/ea...

I'll launch this as a Finch trial so we can see the impact on those UMAs.

[jochen (gone - plz use gerrit), 2016-08-26 15:01:58]

lgtm

[Sami, 2016-08-26 15:54:18]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2016-08-26 15:54:35]

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-08-26 18:33:51]

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

[commit-bot: I haz the power, 2016-08-26 18:33:52]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Sami, 2016-08-30 09:46:33]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2016-08-30 09:46:34]

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

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

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

[Sami, 2016-08-30 09:46:46]

The CQ bit was checked by skyostil@chromium.org

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

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

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

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

[Sami, 2016-08-30 11:08:59]

The CQ bit was checked by skyostil@chromium.org

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

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-08-30 12:44:57]

Description was changed from

==========
scheduler: Throttle timers in out-of-view frames

Throttle timer tasks to 1 Hz in out-of-view or invisible frames. This
reduces the power consumption and performance impact of these tasks,
which are not likely having any visible effect on the page anyway.

BUG=616519
==========

to

==========
scheduler: Throttle timers in out-of-view frames

Throttle timer tasks to 1 Hz in out-of-view or invisible frames. This
reduces the power consumption and performance impact of these tasks,
which are not likely having any visible effect on the page anyway.

BUG=616519
==========

[commit-bot: I haz the power, 2016-08-30 12:44:59]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-30 12:47:39]

Description was changed from

==========
scheduler: Throttle timers in out-of-view frames

Throttle timer tasks to 1 Hz in out-of-view or invisible frames. This
reduces the power consumption and performance impact of these tasks,
which are not likely having any visible effect on the page anyway.

BUG=616519
==========

to

==========
scheduler: Throttle timers in out-of-view frames

Throttle timer tasks to 1 Hz in out-of-view or invisible frames. This
reduces the power consumption and performance impact of these tasks,
which are not likely having any visible effect on the page anyway.

BUG=616519

Committed: https://crrev.com/7fec349e59e40dd9beee5f7eafa9e0db5c4ac1bd
Cr-Commit-Position: refs/heads/master@{#415278}
==========

[commit-bot: I haz the power, 2016-08-30 12:47:40]

Patchset 8 (id:??) landed as
https://crrev.com/7fec349e59e40dd9beee5f7eafa9e0db5c4ac1bd
Cr-Commit-Position: refs/heads/master@{#415278}