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>
Created attachment 231040 [details] patch
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 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 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.
(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.
http://trac.webkit.org/changeset/168493
Crash fix in http://trac.webkit.org/changeset/168509
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.
(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.
(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.
This also caused https://bugs.webkit.org/show_bug.cgi?id=132783.