Bug 132794 – REGRESSION (iOS WebKit2): requestAnimationFrame fires more than once between layer tree commits

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

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.

[Radar WebKit Bug Importer]

<rdar://problem/16877909>

[Tim Horton]

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%

[Tim Horton]

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.

[Tim Horton]

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

[Tim Horton]

Created attachment 231999 [details]
patch

[WebKit Commit Bot]

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.

[Simon Fraser (smfr)]

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?

[Tim Horton]

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.

[Tim Horton]

Created attachment 232011 [details]
for ews

[Tim Horton]

Created attachment 232012 [details]
for ews

[Tim Horton]

http://trac.webkit.org/changeset/169299