Clean up the layer volatility code and logging
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Jul 2018 21:04:10 +0000 (21:04 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Jul 2018 21:04:10 +0000 (21:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187286

Reviewed by Tim Horton.
Source/WebCore:

Export a function.

* platform/graphics/cocoa/IOSurface.h:

Source/WebKit:

Fix the layer volatility logging so it doesn't say "succeeded" when it actually failed
and gave up.

Use a couple of lambda functions in RemoteLayerBackingStore::setBufferVolatility() to
make the code easier to read.

* Shared/RemoteLayerTree/RemoteLayerBackingStore.h:
* Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:
(WebKit::RemoteLayerBackingStore::setBufferVolatility):
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::callVolatilityCompletionHandlers):
(WebKit::WebPage::layerVolatilityTimerFired):
(WebKit::WebPage::markLayersVolatile):
* WebProcess/WebPage/WebPage.h:
(WebKit::WebPage::markLayersVolatile):
* WebProcess/WebProcess.cpp:
(WebKit::WebProcess::actualPrepareToSuspend):
(WebKit::WebProcess::markAllLayersVolatile):
* WebProcess/WebProcess.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@233479 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/cocoa/IOSurface.h
Source/WebKit/ChangeLog
Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h
Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm
Source/WebKit/WebProcess/WebPage/WebPage.cpp
Source/WebKit/WebProcess/WebPage/WebPage.h
Source/WebKit/WebProcess/WebProcess.cpp
Source/WebKit/WebProcess/WebProcess.h

index 4de87a8..1b02f39 100644 (file)
@@ -1,3 +1,14 @@
+2018-07-02  Simon Fraser  <simon.fraser@apple.com>
+
+        Clean up the layer volatility code and logging
+        https://bugs.webkit.org/show_bug.cgi?id=187286
+
+        Reviewed by Tim Horton.
+
+        Export a function.
+
+        * platform/graphics/cocoa/IOSurface.h:
+
 2018-07-03  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r233112.
index d180eb4..3f68ecf 100644 (file)
@@ -130,7 +130,7 @@ public:
 
     CGColorSpaceRef colorSpace() const { return m_colorSpace.get(); }
     WEBCORE_EXPORT Format format() const;
-    IOSurfaceID surfaceID() const;
+    WEBCORE_EXPORT IOSurfaceID surfaceID() const;
     size_t bytesPerRow() const;
 
     WEBCORE_EXPORT bool isInUse() const;
index f65f4e2..46cc6ca 100644 (file)
@@ -1,3 +1,30 @@
+2018-07-02  Simon Fraser  <simon.fraser@apple.com>
+
+        Clean up the layer volatility code and logging
+        https://bugs.webkit.org/show_bug.cgi?id=187286
+
+        Reviewed by Tim Horton.
+        
+        Fix the layer volatility logging so it doesn't say "succeeded" when it actually failed
+        and gave up.
+        
+        Use a couple of lambda functions in RemoteLayerBackingStore::setBufferVolatility() to
+        make the code easier to read.
+
+        * Shared/RemoteLayerTree/RemoteLayerBackingStore.h:
+        * Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:
+        (WebKit::RemoteLayerBackingStore::setBufferVolatility):
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::callVolatilityCompletionHandlers):
+        (WebKit::WebPage::layerVolatilityTimerFired):
+        (WebKit::WebPage::markLayersVolatile):
+        * WebProcess/WebPage/WebPage.h:
+        (WebKit::WebPage::markLayersVolatile):
+        * WebProcess/WebProcess.cpp:
+        (WebKit::WebProcess::actualPrepareToSuspend):
+        (WebKit::WebProcess::markAllLayersVolatile):
+        * WebProcess/WebProcess.h:
+
 2018-07-03  John Wilander  <wilander@apple.com>
 
         Resource Load Statistics: Make WebsiteDataStore::getAllStorageAccessEntries() call the right network process instead of iterating over the process pools
index d86601e..a99e9c5 100644 (file)
@@ -90,6 +90,7 @@ public:
         SecondaryBack
     };
 
+    // Returns true if it was able to fulfill the request. This can fail when trying to mark an in-use surface as volatile.
     bool setBufferVolatility(BufferType, bool isVolatile);
 
     MonotonicTime lastDisplayTime() const { return m_lastDisplayTime; }
index b9a6afe..2d74545 100644 (file)
@@ -430,43 +430,53 @@ RetainPtr<CGContextRef> RemoteLayerBackingStore::takeFrontContextPendingFlush()
 #if HAVE(IOSURFACE)
 bool RemoteLayerBackingStore::setBufferVolatility(BufferType type, bool isVolatile)
 {
+    // Return value is true if we succeeded in making volatile.
+    auto makeVolatile = [] (Buffer& buffer) -> bool {
+        if (!buffer.surface || buffer.isVolatile)
+            return true;
+
+        buffer.surface->releaseGraphicsContext();
+
+        if (!buffer.surface->isInUse()) {
+            buffer.surface->setIsVolatile(true);
+            buffer.isVolatile = true;
+            return true;
+        }
+    
+        return false;
+    };
+
+    // Return value is true if we need to repaint.
+    auto makeNonVolatile = [] (Buffer& buffer) -> bool {
+        if (!buffer.surface || !buffer.isVolatile)
+            return false;
+
+        auto previousState = buffer.surface->setIsVolatile(false);
+        buffer.isVolatile = false;
+
+        return previousState == WebCore::IOSurface::SurfaceState::Empty;
+    };
+
     switch (type) {
     case BufferType::Front:
-        if (m_frontBuffer.surface && m_frontBuffer.isVolatile != isVolatile) {
-            if (isVolatile)
-                m_frontBuffer.surface->releaseGraphicsContext();
-            if (!isVolatile || !m_frontBuffer.surface->isInUse()) {
-                auto previousState = m_frontBuffer.surface->setIsVolatile(isVolatile);
-                m_frontBuffer.isVolatile = isVolatile;
-
-                // Becoming non-volatile and the front buffer was purged, so we need to repaint.
-                if (!isVolatile && (previousState == WebCore::IOSurface::SurfaceState::Empty))
-                    setNeedsDisplay();
-            } else
-                return false;
-        }
+        if (isVolatile)
+            return makeVolatile(m_frontBuffer);
+        
+        // Becoming non-volatile and the front buffer was purged, so we need to repaint.
+        if (makeNonVolatile(m_frontBuffer))
+            setNeedsDisplay();
         break;
     case BufferType::Back:
-        if (m_backBuffer.surface && m_backBuffer.isVolatile != isVolatile) {
-            if (isVolatile)
-                m_backBuffer.surface->releaseGraphicsContext();
-            if (!isVolatile || !m_backBuffer.surface->isInUse()) {
-                m_backBuffer.surface->setIsVolatile(isVolatile);
-                m_backBuffer.isVolatile = isVolatile;
-            } else
-                return false;
-        }
+        if (isVolatile)
+            return makeVolatile(m_backBuffer);
+    
+        makeNonVolatile(m_backBuffer);
         break;
     case BufferType::SecondaryBack:
-        if (m_secondaryBackBuffer.surface && m_secondaryBackBuffer.isVolatile != isVolatile) {
-            if (isVolatile)
-                m_secondaryBackBuffer.surface->releaseGraphicsContext();
-            if (!isVolatile || !m_secondaryBackBuffer.surface->isInUse()) {
-                m_secondaryBackBuffer.surface->setIsVolatile(isVolatile);
-                m_secondaryBackBuffer.isVolatile = isVolatile;
-            } else
-                return false;
-        }
+        if (isVolatile)
+            return makeVolatile(m_secondaryBackBuffer);
+    
+        makeNonVolatile(m_secondaryBackBuffer);
         break;
     }
     return true;
index fdd4dbe..a3949ea 100644 (file)
@@ -2219,11 +2219,11 @@ void WebPage::setLayerTreeStateIsFrozen(bool frozen)
     drawingArea->setLayerTreeStateIsFrozen(frozen);
 }
 
-void WebPage::callVolatilityCompletionHandlers()
+void WebPage::callVolatilityCompletionHandlers(bool succeeded)
 {
     auto completionHandlers = WTFMove(m_markLayersAsVolatileCompletionHandlers);
     for (auto& completionHandler : completionHandlers)
-        completionHandler();
+        completionHandler(succeeded);
 }
 
 void WebPage::layerVolatilityTimerFired()
@@ -2232,12 +2232,15 @@ void WebPage::layerVolatilityTimerFired()
     bool didSucceed = markLayersVolatileImmediatelyIfPossible();
     if (didSucceed || newInterval > maximumLayerVolatilityTimerInterval) {
         m_layerVolatilityTimer.stop();
-        RELEASE_LOG_IF_ALLOWED("%p - WebPage - Attempted to mark layers as volatile, success? %d", this, didSucceed);
-        callVolatilityCompletionHandlers();
+        if (didSucceed)
+            RELEASE_LOG_IF_ALLOWED("%p - WebPage - Succeeded in marking layers as volatile", this);
+        else
+            RELEASE_LOG_IF_ALLOWED("%p - WebPage - Failed to mark layers as volatile within %gms", this, maximumLayerVolatilityTimerInterval.milliseconds());
+        callVolatilityCompletionHandlers(didSucceed);
         return;
     }
 
-    RELEASE_LOG_ERROR_IF_ALLOWED("%p - WebPage - Failed to mark all layers as volatile, will retry in %g ms", this, newInterval.value() * 1000);
+    RELEASE_LOG_ERROR_IF_ALLOWED("%p - WebPage - Failed to mark all layers as volatile, will retry in %g ms", this, newInterval.milliseconds());
     m_layerVolatilityTimer.startRepeating(newInterval);
 }
 
@@ -2246,7 +2249,7 @@ bool WebPage::markLayersVolatileImmediatelyIfPossible()
     return !drawingArea() || drawingArea()->markLayersVolatileImmediatelyIfPossible();
 }
 
-void WebPage::markLayersVolatile(WTF::Function<void ()>&& completionHandler)
+void WebPage::markLayersVolatile(WTF::Function<void (bool)>&& completionHandler)
 {
     RELEASE_LOG_IF_ALLOWED("%p - WebPage::markLayersVolatile()", this);
 
@@ -2264,11 +2267,11 @@ void WebPage::markLayersVolatile(WTF::Function<void ()>&& completionHandler)
             // If we get suspended when locking the screen, it is expected that some IOSurfaces cannot be marked as purgeable so we do not keep retrying.
             RELEASE_LOG_IF_ALLOWED("%p - WebPage - Did what we could to mark IOSurfaces as purgeable after locking the screen", this);
         }
-        callVolatilityCompletionHandlers();
+        callVolatilityCompletionHandlers(didSucceed);
         return;
     }
 
-    RELEASE_LOG_IF_ALLOWED("%p - Failed to mark all layers as volatile, will retry in %g ms", this, initialLayerVolatilityTimerInterval.value() * 1000);
+    RELEASE_LOG_IF_ALLOWED("%p - Failed to mark all layers as volatile, will retry in %g ms", this, initialLayerVolatilityTimerInterval.milliseconds());
     m_layerVolatilityTimer.startRepeating(initialLayerVolatilityTimerInterval);
 }
 
index 1dbecf3..741e92f 100644 (file)
@@ -669,7 +669,7 @@ public:
     bool hasRichlyEditableSelection() const;
 
     void setLayerTreeStateIsFrozen(bool);
-    void markLayersVolatile(WTF::Function<void ()>&& completionHandler = { });
+    void markLayersVolatile(WTF::Function<void (bool)>&& completionHandler = { });
     void cancelMarkLayersVolatile();
 
     NotificationPermissionRequestManager* notificationPermissionRequestManager();
@@ -1146,7 +1146,7 @@ private:
 
     bool markLayersVolatileImmediatelyIfPossible();
     void layerVolatilityTimerFired();
-    void callVolatilityCompletionHandlers();
+    void callVolatilityCompletionHandlers(bool succeeded);
 
     String sourceForFrame(WebFrame*);
 
@@ -1669,7 +1669,7 @@ private:
 #endif
 
     WebCore::Timer m_layerVolatilityTimer;
-    Vector<WTF::Function<void ()>> m_markLayersAsVolatileCompletionHandlers;
+    Vector<WTF::Function<void (bool)>> m_markLayersAsVolatileCompletionHandlers;
     bool m_isSuspendedUnderLock { false };
 
     HashSet<String, ASCIICaseInsensitiveHash> m_mimeTypesWithCustomContentProviders;
index 0066f11..7db4e31 100644 (file)
@@ -1375,8 +1375,11 @@ void WebProcess::actualPrepareToSuspend(ShouldAcknowledgeWhenReadyToSuspend shou
     accessibilityProcessSuspendedNotification(true);
 #endif
 
-    markAllLayersVolatile([this, shouldAcknowledgeWhenReadyToSuspend] {
-        RELEASE_LOG(ProcessSuspension, "%p - WebProcess::markAllLayersVolatile() Successfuly marked all layers as volatile", this);
+    markAllLayersVolatile([this, shouldAcknowledgeWhenReadyToSuspend](bool success) {
+        if (success)
+            RELEASE_LOG(ProcessSuspension, "%p - WebProcess::markAllLayersVolatile() Successfuly marked all layers as volatile", this);
+        else
+            RELEASE_LOG(ProcessSuspension, "%p - WebProcess::markAllLayersVolatile() Failed to mark all layers as volatile", this);
 
         if (shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) {
             RELEASE_LOG(ProcessSuspension, "%p - WebProcess::actualPrepareToSuspend() Sending ProcessReadyToSuspend IPC message", this);
@@ -1427,20 +1430,25 @@ void WebProcess::cancelPrepareToSuspend()
     parentProcessConnection()->send(Messages::WebProcessProxy::DidCancelProcessSuspension(), 0);
 }
 
-void WebProcess::markAllLayersVolatile(WTF::Function<void()>&& completionHandler)
+void WebProcess::markAllLayersVolatile(WTF::Function<void(bool)>&& completionHandler)
 {
     RELEASE_LOG(ProcessSuspension, "%p - WebProcess::markAllLayersVolatile()", this);
     ASSERT(!m_pageMarkingLayersAsVolatileCounter);
+    m_countOfPagesFailingToMarkVolatile = 0;
+
     m_pageMarkingLayersAsVolatileCounter = std::make_unique<PageMarkingLayersAsVolatileCounter>([this, completionHandler = WTFMove(completionHandler)] (RefCounterEvent) {
         if (m_pageMarkingLayersAsVolatileCounter->value())
             return;
 
-        completionHandler();
+        completionHandler(m_countOfPagesFailingToMarkVolatile == 0);
         m_pageMarkingLayersAsVolatileCounter = nullptr;
     });
     auto token = m_pageMarkingLayersAsVolatileCounter->count();
     for (auto& page : m_pageMap.values())
-        page->markLayersVolatile([token] { });
+        page->markLayersVolatile([token, this] (bool succeeded) {
+            if (!succeeded)
+                ++m_countOfPagesFailingToMarkVolatile;
+        });
 }
 
 void WebProcess::cancelMarkAllLayersVolatile()
index 88cc21f..9808702 100644 (file)
@@ -252,7 +252,7 @@ private:
     void registerWithStateDumper();
 #endif
 
-    void markAllLayersVolatile(WTF::Function<void()>&& completionHandler);
+    void markAllLayersVolatile(WTF::Function<void(bool)>&& completionHandler);
     void cancelMarkAllLayersVolatile();
     void setAllLayerTreeStatesFrozen(bool);
     void processSuspensionCleanupTimerFired();
@@ -448,6 +448,7 @@ private:
     enum PageMarkingLayersAsVolatileCounterType { };
     using PageMarkingLayersAsVolatileCounter = RefCounter<PageMarkingLayersAsVolatileCounterType>;
     std::unique_ptr<PageMarkingLayersAsVolatileCounter> m_pageMarkingLayersAsVolatileCounter;
+    unsigned m_countOfPagesFailingToMarkVolatile { 0 };
 
     bool m_suppressMemoryPressureHandler { false };
 #if PLATFORM(MAC)