Bug 132667 - [iOS WebKit2] Flush RemoteLayerBackingStore contexts on a secondary queue
Summary: [iOS WebKit2] Flush RemoteLayerBackingStore contexts on a secondary queue
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-07 17:31 PDT by Tim Horton
Modified: 2014-05-10 14:21 PDT (History)
6 users (show)

See Also:


Attachments
patch (10.65 KB, patch)
2014-05-07 21:08 PDT, Tim Horton
darin: review+
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-07 17:31:06 PDT
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>
Comment 1 Tim Horton 2014-05-07 21:08:02 PDT
Created attachment 231040 [details]
patch
Comment 2 WebKit Commit Bot 2014-05-07 21:10:37 PDT
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.
Comment 3 Anders Carlsson 2014-05-08 08:15:23 PDT
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.
Comment 4 Darin Adler 2014-05-08 12:32:33 PDT
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.
Comment 5 Tim Horton 2014-05-08 13:22:41 PDT
(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.
Comment 6 Tim Horton 2014-05-08 13:57:25 PDT
http://trac.webkit.org/changeset/168493
Comment 7 Tim Horton 2014-05-08 17:22:46 PDT
Crash fix in http://trac.webkit.org/changeset/168509
Comment 8 Darin Adler 2014-05-09 07:38:19 PDT
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.
Comment 9 Tim Horton 2014-05-09 12:27:22 PDT
(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.
Comment 10 Tim Horton 2014-05-09 12:32:36 PDT
(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.
Comment 11 Tim Horton 2014-05-10 14:21:48 PDT
This also caused https://bugs.webkit.org/show_bug.cgi?id=132783.