Don't fire requestAnimationFrame for scripts in frames that are out of view

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Nick Johnson]

Don't fire requestAnimationFrame for scripts in frames that are out of view

[Nick Johnson]

Created attachment 170408 [details]
Patch

[Nick Johnson]

When a frame is out of view (such as an iframe that's scrolled offscreen), WebKit shouldn't fire requestAnimationFrame callbacks.

Avoiding these unnecessary callbacks reduces CPU consumption, and thus, battery consumption on mobile devices.

The attached patch implements that optimisation.

[WebKit Review Bot]

Comment on attachment 170408 [details]
Patch

Attachment 170408 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14563259

New failing tests:
fast/animation/request-animation-frame-offscreen-iframe.html
fast/animation/request-animation-frame-in-scrolled-div.html
fast/animation/request-animation-frame-iframe.html
fast/animation/request-animation-frame-iframe2.html

[Dean Jackson]

Comment on attachment 170408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170408&action=review

This looks ok to me, but I'm not sure why some older tests are failing.

> LayoutTests/ChangeLog:12
> +    When a frame is out of view (such as an iframe that's scrolled
> +    offscreen), WebKit shouldn't fire requestAnimationFrame callbacks.
> +
> +    Avoiding these unnecessary callbacks reduces CPU consumption, and thus,
> +    battery consumption on mobile devices.

Needs more indentation.

[Dean Jackson]

Comment on attachment 170408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170408&action=review

> LayoutTests/fast/animation/request-animation-frame-in-scrolled-div.html:24
> +                if (window.testRunner)
> +                testRunner.display();

Indentation broken.

> LayoutTests/fast/animation/request-animation-frame-in-scrolled-div.html:41
> +                if (window.testRunner)
> +                testRunner.notifyDone();

Indentation broken.

[Nick Johnson]

I'll fix those and resubmit. The tests all pass on my machine (OS X 10.7.5); I'm currently trying (without much success) to build the chromium port of Webkit to try and replicate the failure buildbot is seeing. Any insight anyone can provide would be greatly appreciated, since the buildbot doesn't seem to show test output anywhere.

[Nick Johnson]

Created attachment 171278 [details]
Patch

[Darin Adler]

Comment on attachment 171278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171278&action=review

> Source/WebCore/dom/ScriptedAnimationController.cpp:123
> +    if (view && view->windowClipRect(false).isEmpty()) {
> +        scheduleAnimation();
> +        return;
> +    }

Two things seem not quite right to me.

1)  I don’t think that windowClipRect(false).isEmpty() is the correct visibility check; I can’t find any other code that’s using that and this can’t be the only place we care about visibility.
2) I don’t think that polling is the right way to detect when the frame becomes visible again. We want to actively trigger it rather than running the timer over and over again.

[Nick Johnson]

(In reply to comment #8)
> (From update of attachment 171278 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171278&action=review
> 
> > Source/WebCore/dom/ScriptedAnimationController.cpp:123
> > +    if (view && view->windowClipRect(false).isEmpty()) {
> > +        scheduleAnimation();
> > +        return;
> > +    }
> 
> Two things seem not quite right to me.
> 
> 1)  I don’t think that windowClipRect(false).isEmpty() is the correct visibility check; I can’t find any other code that’s using that and this can’t be the only place we care about visibility.

I spent a lot of time reading the code to try and understand the rendering process; I found windowClipRect in use in ScrollView::scrollContents to determine the visible part of the window. If there's a better method to use, I'm happy to change it; I just wasn't able to find a better choice myself.

> 2) I don’t think that polling is the right way to detect when the frame becomes visible again. We want to actively trigger it rather than running the timer over and over again.

I agree entirely, but based on my search of the code concerned with scrolling and rendering, there's currently nothing that triggers events when elements become visible or invisible, and no facility to trigger those events. Adding it would undoubtedly be possible, but I suspect it would be a significant and much higher impact (and risk) change.

The polling approach, while slightly messy, is an easy win - the timer is running repeatedly already, the patch simply avoids the expensive JavaScript callback when it's unnecessary - and seems unlikely to have unexpected side effects. In my experiments with content rendered in an iframe using requestAnimationFrame, CPU usage drops from anything up to 100% to effectively zero when the frame is scrolled offscreen.

[WebKit Review Bot]

Comment on attachment 171278 [details]
Patch

Attachment 171278 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14641174

New failing tests:
fast/animation/request-animation-frame-offscreen-iframe.html
fast/animation/request-animation-frame-in-scrolled-div.html
fast/animation/request-animation-frame-iframe.html
fast/animation/request-animation-frame-iframe2.html

[Nick Johnson]

(In reply to comment #10)
> (From update of attachment 171278 [details])
> Attachment 171278 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14641174
> 
> New failing tests:
> fast/animation/request-animation-frame-offscreen-iframe.html
> fast/animation/request-animation-frame-in-scrolled-div.html
> fast/animation/request-animation-frame-iframe.html
> fast/animation/request-animation-frame-iframe2.html

These tests pass on my mac, and I've now built the Chromium port of Webkit on Linux (Ubuntu 12.10), where they also pass. Is anyone able to provide insight into why they are failing on buildbot?

[Nick Johnson]

I've tried everything I can think of to reproduce the failures buildbot is seeing, without success. The suggestion on the webkit-dev mailing list is to land the patch and take care of any failures if they arise on the build console, where the details of the failures will be visible.

If there's anything else I need to do in order to get this patch approved and landed, please let me know.

[Darin Adler]

Comment on attachment 171278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171278&action=review

>>> Source/WebCore/dom/ScriptedAnimationController.cpp:123
>>> +    }
>> 
>> Two things seem not quite right to me.
>> 
>> 1)  I don’t think that windowClipRect(false).isEmpty() is the correct visibility check; I can’t find any other code that’s using that and this can’t be the only place we care about visibility.
>> 2) I don’t think that polling is the right way to detect when the frame becomes visible again. We want to actively trigger it rather than running the timer over and over again.
> 
> I spent a lot of time reading the code to try and understand the rendering process; I found windowClipRect in use in ScrollView::scrollContents to determine the visible part of the window. If there's a better method to use, I'm happy to change it; I just wasn't able to find a better choice myself.

OK. That’s a fine explanation for why you did what you did, but not reason to land this patch as-is. We need to find others expert on this enough that they can help us find the right way to do it.

We do want this behavior change, but we need a correct implementation.

[Nick Johnson]

Landing this patch would not preclude landing a later one that implements an event driven model. The patch is not incorrect; it's simply the case that there exists a potential implementation that is more efficient.

Is it your position that it's better to have no patch than this one? It seems to me that a better approach would be to land this patch now, and implement a better solution later, since this patch provides a performance improvement - just one that isn't as great as it could potentially be.

[Simon Fraser (smfr)]

(In reply to comment #14)
> Is it your position that it's better to have no patch than this one? It seems to me that a better approach would be to land this patch now, and implement a better solution later, since this patch provides a performance improvement - just one that isn't as great as it could potentially be.

Do you know of any actual pages that would benefit from this performance improvement?

What does the page visibility API do for out-of-view iframes?

[Nick Johnson]

(In reply to comment #15)
> (In reply to comment #14)
> > Is it your position that it's better to have no patch than this one? It seems to me that a better approach would be to land this patch now, and implement a better solution later, since this patch provides a performance improvement - just one that isn't as great as it could potentially be.
> 
> Do you know of any actual pages that would benefit from this performance improvement?

In practice, there are few pages currently using requestAnimationFrame, since it's not widespread yet.

In principle, any animated advert in an IFrame (which is most of them) would benefit from this if the ad used requestAnimationFrame.

> 
> What does the page visibility API do for out-of-view iframes?

Per the spec (http://www.w3.org/TR/page-visibility/#sec-document-interface):

"On getting, the hidden attribute MUST return true if the Document contained by the top level browsing context (root window in the browser's viewport) [HTML5] is not visible at all. The attribute MUST return false if the Document contained by the top level browsing context is at least partially visible on at least one screen."

So as it stands, it conveys visibility information only for the top level document, not for any nested documents. I don't believe there's any assumption that the Document.hidden property will correlate with the status of a requestAnimationFrame callback anywhere, though.

[Maciej Stachowiak]

(In reply to comment #16)
> (In reply to comment #15)

> > 
> > What does the page visibility API do for out-of-view iframes?
> 
> Per the spec (http://www.w3.org/TR/page-visibility/#sec-document-interface):
> 
> "On getting, the hidden attribute MUST return true if the Document contained by the top level browsing context (root window in the browser's viewport) [HTML5] is not visible at all. The attribute MUST return false if the Document contained by the top level browsing context is at least partially visible on at least one screen."
> 
> So as it stands, it conveys visibility information only for the top level document, not for any nested documents. I don't believe there's any assumption that the Document.hidden property will correlate with the status of a requestAnimationFrame callback anywhere, though.

Is this patch consistent with the RAF spec? It says:

<http://www.w3.org/TR/animation-timing/>
"Whenever a Document's hidden attribute is false and the animation frame request callback list is not empty, the user agent MUST regularly queue a task that samples all animations for that Document's top-level browsing context."

So taken literally, it does not seem to allow the behavior of this patch.

[Simon Fraser (smfr)]

I think we fixed this already.

[Chris Dumez]

*** This bug has been marked as a duplicate of bug 144718 ***