Bug 130768 - [iOS WebKit2] Flush all surfaces after painting into all of them, instead of after painting into each one
Summary: [iOS WebKit2] Flush all surfaces after painting into all of them, instead of ...
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: 130822
Blocks:
  Show dependency tree / graph
 
Reported: 2014-03-25 23:49 PDT by Tim Horton
Modified: 2014-03-31 16:31 PDT (History)
4 users (show)

See Also:

Attachments
patch (11.14 KB, patch)
2014-03-26 00:57 PDT, Tim Horton 		simon.fraser: review+
Details | Formatted Diff | Diff
patch (24.48 KB, patch)
2014-03-27 01:56 PDT, Tim Horton 		no flags Details | Formatted Diff | Diff
patch (will break clones but fix in a follow-up) (19.89 KB, patch)
2014-03-31 15:23 PDT, Tim Horton 		no flags Details | Formatted Diff | Diff
maybe rebase (19.00 KB, patch)
2014-03-31 16:02 PDT, Tim Horton 		benjamin: review+
Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2014-03-25 23:49:30 PDT 
The remote layer tree is effectively painting layers serially at the moment, as RemoteLayerBackingStore::drawInContext first paints into the layer, then flushes it.

To increase parallelism and hopefully increase painting performance, we should first paint all of the changed backing stores, then flush them all.

Ideally in the future we'd move that flush off onto another thread so we can move forward with the next frame while flushing, but that's for another patch.

<rdar://problem/16421471>
Comment 1 Tim Horton 2014-03-26 00:57:51 PDT 
Created attachment 227832 [details]
patch
Comment 2 Tim Horton 2014-03-26 08:26:14 PDT 
http://trac.webkit.org/changeset/166296
Comment 3 WebKit Commit Bot 2014-03-27 00:09:19 PDT 
Re-opened since this is blocked by bug 130822
Comment 4 Tim Horton 2014-03-27 01:56:17 PDT 
Created attachment 227934 [details]
patch
Comment 5 Tim Horton 2014-03-27 01:56:41 PDT 
I need to double check what happens with clones.
Comment 6 Tim Horton 2014-03-27 13:24:41 PDT 
Comment on attachment 227934 [details]
patch

Clones are broken.
Comment 7 Tim Horton 2014-03-31 15:23:37 PDT 
Created attachment 228210 [details]
patch (will break clones but fix in a follow-up)
Comment 8 Tim Horton 2014-03-31 16:02:42 PDT 
Created attachment 228213 [details]
maybe rebase
Comment 9 Benjamin Poulain 2014-03-31 16:05:48 PDT 
View in context: https://bugs.webkit.org/attachment.cgi?id=228210&action=review

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.h:46
>  class RemoteLayerBackingStore {
> +    WTF_MAKE_NONCOPYABLE(RemoteLayerBackingStore);

How come this is not Fast Allocated?

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:337
> +    if (!m_frontContextPendingFlush)
> +        return;
> +
> +    CGContextFlush(m_frontContextPendingFlush.get());
> +    m_frontContextPendingFlush = nullptr;

Maybe?

if (m_frontContextPendingFlush) {
    CGContextFlush(m_frontContextPendingFlush.get());
    m_frontContextPendingFlush = nullptr;
}

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:418
> +    for (const auto& layer : m_changedLayers) {

"auto" sucks here, no way to guess the type.

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemote.h:154
> +    RemoteLayerTreeTransaction::LayerProperties& properties() { return m_properties; }

You could have a const version.

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:310
> +    for (auto& layer : transaction.changedLayers()) {

auto sucks here too :(
Comment 10 Benjamin Poulain 2014-03-31 16:06:20 PDT 
Comment on attachment 228213 [details]
maybe rebase

r+ with the comments from the previous patch.
Comment 11 Tim Horton 2014-03-31 16:31:22 PDT 
http://trac.webkit.org/changeset/166545