Bug 132794 - REGRESSION (iOS WebKit2): requestAnimationFrame fires more than once between layer tree commits
Summary: REGRESSION (iOS WebKit2): requestAnimationFrame fires more than once between ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-05-10 17:31 PDT by Tim Horton
Modified: 2014-05-23 19:19 PDT (History)
12 users (show)

See Also:


Attachments
patch (82.01 KB, patch)
2014-05-23 16:04 PDT, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff
for ews (80.64 KB, patch)
2014-05-23 18:57 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
for ews (80.93 KB, patch)
2014-05-23 19:10 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2014-05-10 17:31:32 PDT
Because remote layer tree flushing is decoupled from CoreAnimation/render server commits, the display link callback that we use to drive requestAnimation frame can fire more than once per frame-that-actually-makes-it-to-the-screen.

This means two things:

1) Benchmarks are broken, because they use the rate of rAF to determine how responsive painting is/how much their benchmarking is slowing things down.
2) We can be doing a lot (*lot*, in the case of canvas) of unnecessary work in rAF callbacks that just ends up getting thrown away/never makes it to the screen.

Running http://www.kevs3d.co.uk/dev/canvasmark/ I see between 1 and 3 callbacks per frame, but I don't think there's any reason it couldn't be more than that.
Comment 1 Radar WebKit Bug Importer 2014-05-10 17:31:48 PDT
<rdar://problem/16877909>
Comment 2 Tim Horton 2014-05-10 17:53:25 PDT
This makes http://www.kevs3d.co.uk/dev/canvasmark/ much, much smoother (to the eye), and drops Web process JSC time (including time under the rAF callback) by about 30%
Comment 3 Tim Horton 2014-05-10 17:55:18 PDT
By "this", I mean: I hacked it up so that we send a rAF callback from RemoteLayerTreeDrawingArea::didUpdate(), which comes from the UI process after it commits the previous frame. It's a bit of a hack, and not the ideal fix (it means the rAF callback has nothing to do with the screen/window server, and more to do with the Web process' rendering -- maybe that's a good thing?), but I'm less sure what the correct fix is.
Comment 4 Tim Horton 2014-05-10 17:59:33 PDT
One obvious problem with that approach is that if the rAF callback doesn't cause a layer tree update, we'll never get another rAF callback (until someone else causes a layer tree update, either via user interaction or a timer or whatever).
Comment 5 Tim Horton 2014-05-23 16:04:09 PDT
Created attachment 231999 [details]
patch
Comment 6 WebKit Commit Bot 2014-05-23 16:05:05 PDT
Attachment 231999 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:45:  The parameter name "factory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:63:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:87:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/ios/DisplayRefreshMonitorIOS.mm:97:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/WebProcess/WebPage/Cocoa/RemoteLayerTreeDisplayRefreshMonitor.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebKit2/WebProcess/WebPage/Cocoa/RemoteLayerTreeDisplayRefreshMonitor.h:50:  The parameter name "drawingArea" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Simon Fraser (smfr) 2014-05-23 16:46:33 PDT
Comment on attachment 231999 [details]
patch

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

> Source/WebCore/dom/ScriptedAnimationController.h:95
> +    virtual DisplayRefreshMonitorFactory* displayRefreshMonitorFactory() const override;

Do we really need a factory class, or can we just ask it to create a DisplayRefreshMonitor?

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:42
> +    if (factory)
> +        return factory->createDisplayRefreshMonitor(displayID);

It's a bit weird for the create() method to just turn around and use the factory passed in.

> Source/WebCore/platform/graphics/DisplayRefreshMonitorClient.cpp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

2014?

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:63
> +    if (!client->m_displayIDIsSet)

Weird to access internal state.

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:71
> +    if (!client->m_displayIDIsSet)

Ditto.

> Source/WebCore/platform/graphics/GraphicsLayerUpdater.cpp:86
> +DisplayRefreshMonitorFactory* GraphicsLayerUpdater::displayRefreshMonitorFactory() const
> +{
> +    return m_client ? m_client->displayRefreshMonitorFactory() : nullptr;
> +}
> +#endif

Maybe push the 82DisplayRefreshMonitor onto the GraphicsLayerUpdater?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3937
> +DisplayRefreshMonitorFactory* RenderLayerCompositor::displayRefreshMonitorFactory() const
> +{
> +    Frame& frame = m_renderView.frameView().frame();
> +    Page* page = frame.page();
> +    if (!page)
> +        return nullptr;
> +
> +    return page->chrome().client().displayRefreshMonitorFactory();
> +}
> +

Can this go away?

> Source/WebCore/rendering/RenderLayerCompositor.h:392
> +    DisplayRefreshMonitorFactory* displayRefreshMonitorFactory() const;

Why does RLC have to be a factory?
Comment 8 Tim Horton 2014-05-23 17:36:17 PDT
Comment on attachment 231999 [details]
patch

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

>> Source/WebCore/platform/graphics/GraphicsLayerUpdater.cpp:86
>> +#endif
> 
> Maybe push the 82DisplayRefreshMonitor onto the GraphicsLayerUpdater?

No, GraphicsLayerUpdater doesn’t want to know about DisplayRefreshMonitors, it’s just acting as a way for the Manager to get to the ChromeClient associated with the GraphicsLayerUpdater so that the ChromeClient can create the right DisplayRefreshMonitor subclass. Plus, it can have an arbitrary number of DisplayRefreshMonitors.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:3937
>> +
> 
> Can this go away?

Ditto for RLC. I can certainly make this createDisplayRefreshMonitor() instead, but RLC and GraphicsLayerUpdater will still need to be involved.
Comment 9 Tim Horton 2014-05-23 18:57:59 PDT
Created attachment 232011 [details]
for ews
Comment 10 Tim Horton 2014-05-23 19:10:14 PDT
Created attachment 232012 [details]
for ews
Comment 11 Tim Horton 2014-05-23 19:19:31 PDT
http://trac.webkit.org/changeset/169299