Bug 132667 – [iOS WebKit2] Flush RemoteLayerBackingStore contexts on a secondary queue

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Tim Horton]

We can do this expensive and blocking work on a queue, and send the CommitLayerTree message once it's done, and let the Web process continue on with JS or layout or whatever it wants to do (that isn't painting).

<rdar://problem/15349483>

[Tim Horton]

Created attachment 231040 [details]
patch

[WebKit Commit Bot]

Attachment 231040 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:292:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 231040 [details]
patch

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

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:360
> +    RetainPtr<CGContextRef> frontContextPendingFlush = m_frontContextPendingFlush;
> +    m_frontContextPendingFlush = nullptr;

This can just be

RetainPtr<CGContextRef> frontContextPendingFlush = std::move(m_frontContextPendingFlush);

or even

auto frontContextPendingFlush = std::move(m_frontContextPendingFlush);

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:69
> +    m_commitQueue = dispatch_queue_create("com.apple.WebKit.WebContent.RemoteLayerTreeDrawingAreaCommitQueue", nullptr);

I'd put another period before CommitQueue.

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:280
> +            RetainPtr<CGContextRef> contextPendingFlush = layer->properties().backingStore->takeFrontContextPendingFlush();
> +            if (contextPendingFlush)
> +                contextsToFlush.append(contextPendingFlush);

if (auto contextPendingFlush = ...)
    contextToFlush.append(std::move(contextPendingFlush));

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:289
> +    RefPtr<BackingStoreFlusher> backingStoreFlusher = BackingStoreFlusher::create(WebProcess::shared().parentProcessConnection(), std::move(commitEncoder), contextsToFlush);

std::move(contextsToFlush).

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:292
> +    dispatch_async(m_commitQueue, [backingStoreFlusher](){

No need for () in the lambda.

[Darin Adler]

Comment on attachment 231040 [details]
patch

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

>> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:360
>> +    m_frontContextPendingFlush = nullptr;
> 
> This can just be
> 
> RetainPtr<CGContextRef> frontContextPendingFlush = std::move(m_frontContextPendingFlush);
> 
> or even
> 
> auto frontContextPendingFlush = std::move(m_frontContextPendingFlush);

Can it be this?

    return std::move(m_frontContextPendingFlush);

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.h:108
> +        BackingStoreFlusher(IPC::Connection*, std::unique_ptr<IPC::MessageEncoder>, Vector<RetainPtr<CGContextRef>>);

I’m surprised to see both a public constructor and a create function.

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.h:118
> +        std::atomic<bool> m_hasFlushed;

I don’t fully understand the use of std::atomic here. It doesn’t seem sufficient for the cross-thread synchronization I expect would be needed. Maybe it’s just helping the RELEASE_ASSERT work reliably?

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:245
> +    RELEASE_ASSERT(!m_pendingBackingStoreFlusher || m_pendingBackingStoreFlusher->hasFlushed());

Wow, RELEASE_ASSERT. We are getting more liberal with those lately.

What guarantees that the backing store flusher has flushed at this point?

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:322
> +    return adoptRef(new RemoteLayerTreeDrawingArea::BackingStoreFlusher(connection, std::move(encoder), contextsToFlush));

Would a std::move(contextsToFlush) help efficiency here?

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:328
> +    , m_contextsToFlush(contextsToFlush)

Would a std::move(contextsToFlush) here help efficiency?

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:329
> +    , m_hasFlushed(false)

Is this needed? Doesn’t an std::atomic<bool> automatically get initialized to false?

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:339
> +    for (auto& context : m_contextsToFlush)
> +        CGContextFlush(context.get());
> +
> +    m_connection->sendMessage(std::move(m_commitEncoder));
> +    m_hasFlushed = true;

This should either have:

    ASSERT(!m_hasFlushed);

or

    if (m_hasFlushed)
        return;

Or maybe there’s some good reason neither is needed.

[Tim Horton]

(In reply to comment #4)
> (From update of attachment 231040 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231040&action=review
> 
> >> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:360
> >> +    m_frontContextPendingFlush = nullptr;
> > 
> > This can just be
> > 
> > RetainPtr<CGContextRef> frontContextPendingFlush = std::move(m_frontContextPendingFlush);
> > 
> > or even
> > 
> > auto frontContextPendingFlush = std::move(m_frontContextPendingFlush);
> 
> Can it be this?
> 
>     return std::move(m_frontContextPendingFlush);
> 
> > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.h:108
> > +        BackingStoreFlusher(IPC::Connection*, std::unique_ptr<IPC::MessageEncoder>, Vector<RetainPtr<CGContextRef>>);
> 
> I’m surprised to see both a public constructor and a create function.

Whoops!

> > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.h:118
> > +        std::atomic<bool> m_hasFlushed;
> 
> I don’t fully understand the use of std::atomic here. It doesn’t seem sufficient for the cross-thread synchronization I expect would be needed. Maybe it’s just helping the RELEASE_ASSERT work reliably?

Purely for the RELEASE_ASSERT to work reliably; the synchronization is actually performed through another slightly more ridiculous mechanism described below.

> > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:245
> > +    RELEASE_ASSERT(!m_pendingBackingStoreFlusher || m_pendingBackingStoreFlusher->hasFlushed());
> 
> Wow, RELEASE_ASSERT. We are getting more liberal with those lately.
> 
> What guarantees that the backing store flusher has flushed at this point?

The fact that we will bail from flushLayers() until we’ve heard back from the UI process (due to m_waitingForBackingStoreSwap), and we can only hear back from the UI process if we’ve sent it a new layer tree to commit, which happens from inside the backing store flusher!

The RELEASE_ASSERT is simply to ensure that that mechanism is always working (if it fails, we’ll start painting garbage). Getting crash logs in that (very unlikely) case seem much more useful than murmurs of slightly incorrect painting. We’ll keep an eye on it, in any case.

> > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:322
> > +    return adoptRef(new RemoteLayerTreeDrawingArea::BackingStoreFlusher(connection, std::move(encoder), contextsToFlush));
> 
> Would a std::move(contextsToFlush) help efficiency here?
> 
> > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:328
> > +    , m_contextsToFlush(contextsToFlush)
> 
> Would a std::move(contextsToFlush) here help efficiency?
> 
> > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:329
> > +    , m_hasFlushed(false)
> 
> Is this needed? Doesn’t an std::atomic<bool> automatically get initialized to false?

Anders says std::atomic doesn’t initialize.

> > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:339
> > +    for (auto& context : m_contextsToFlush)
> > +        CGContextFlush(context.get());
> > +
> > +    m_connection->sendMessage(std::move(m_commitEncoder));
> > +    m_hasFlushed = true;
> 
> This should either have:
> 
>     ASSERT(!m_hasFlushed);
> 
> or
> 
>     if (m_hasFlushed)
>         return;
> 
> Or maybe there’s some good reason neither is needed.

We make a new one each time through flushLayers(), and only call flush on it once? I’m fine with an ASSERT, though.

[Tim Horton]

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

[Tim Horton]

Crash fix in http://trac.webkit.org/changeset/168509

[Darin Adler]

Comment on attachment 231040 [details]
patch

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

>>> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:322
>>> +    return adoptRef(new RemoteLayerTreeDrawingArea::BackingStoreFlusher(connection, std::move(encoder), contextsToFlush));
>> 
>> Would a std::move(contextsToFlush) help efficiency here?
> 
> Anders says std::atomic doesn’t initialize.

Looking at the patch that was landed, it looks like you missed the comment here about std::move(contextsToFlush), so we’ll be making a copy here.

[Tim Horton]

(In reply to comment #8)
> (From update of attachment 231040 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231040&action=review
> 
> >>> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:322
> >>> +    return adoptRef(new RemoteLayerTreeDrawingArea::BackingStoreFlusher(connection, std::move(encoder), contextsToFlush));
> >> 
> >> Would a std::move(contextsToFlush) help efficiency here?
> > 
> > Anders says std::atomic doesn’t initialize.
> 
> Looking at the patch that was landed, it looks like you missed the comment here about std::move(contextsToFlush), so we’ll be making a copy here.

You're right, I totally missed that one. Will fix.

[Tim Horton]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 231040 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=231040&action=review
> > 
> > >>> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:322
> > >>> +    return adoptRef(new RemoteLayerTreeDrawingArea::BackingStoreFlusher(connection, std::move(encoder), contextsToFlush));
> > >> 
> > >> Would a std::move(contextsToFlush) help efficiency here?
> > > 
> > > Anders says std::atomic doesn’t initialize.
> > 
> > Looking at the patch that was landed, it looks like you missed the comment here about std::move(contextsToFlush), so we’ll be making a copy here.
> 
> You're right, I totally missed that one. Will fix.

Fixed in http://trac.webkit.org/changeset/168544.

[Tim Horton]

This also caused https://bugs.webkit.org/show_bug.cgi?id=132783.