RemoteImageBufferProxyFlushState::waitForDidFlushOnSecondaryThread b…

…locks on a task running on the main thread.

https://bugs.webkit.org/show_bug.cgi?id=256073
<rdar://108642579>

Reviewed by Simon Fraser.

We attempt to wait for flushes of RemoteImageBufferProxy on a background thread, but the 'didFlush' IPC message is received on the main thread.

This can mean it gets delayed (and we don't know the flush is completed) if the main thread is otherwise busy. This delays first-paint on CPLT.

This change passes a Semphore across to the GPUP instead of the flush identifier, and we signal it when the flush is completed rather than sending a return 'didFlush' message.
The WebProcess waits on each flush Semaphore in a background WorkQueue, and then processes the equivalent of 'didFlush' asynchronously and notifies the condition variable to wake any waiting threads.

waitForDidFlushWithTimeout is removed, and all waiters now use the waitForDidFlushOnSecondaryThread code path (renamed) since all waiters are now 'secondary' WRT the flushing WorkQueue.

Synchronous flushes now use a proper synchronous IPC message, rather than relying on async + wait.

* Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.cpp:
(WebKit::RemoteDisplayListRecorder::flushContext):
(WebKit::RemoteDisplayListRecorder::flushContextSync):
* Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.h:
* Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.messages.in:
* Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:
(WebKit::RemoteRenderingBackend::didFlush): Deleted.
* Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h:
* Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.cpp:
(WebKit::RemoteDisplayListRecorderProxy::sendSync):
(WebKit::RemoteDisplayListRecorderProxy::flushContext):
(WebKit::RemoteDisplayListRecorderProxy::flushContextSync):
* Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.h:
* Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.cpp:
(WebKit::RemoteImageBufferProxy::flushDrawingContext):
(WebKit::RemoteImageBufferProxy::flushDrawingContextAsync):
(WebKit::RemoteImageBufferProxyFlushState::waitForDidFlush):
(WebKit::RemoteImageBufferProxy::waitForDidFlushWithTimeout): Deleted.
(WebKit::RemoteImageBufferProxyFlushState::waitForDidFlushOnSecondaryThread): Deleted.
* Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:
* Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:
(WebKit::RemoteRenderingBackendProxy::RemoteRenderingBackendProxy):
(WebKit::RemoteRenderingBackendProxy::disconnectGPUProcess):
(WebKit::RemoteRenderingBackendProxy::addPendingFlush):
(WebKit::RemoteRenderingBackendProxy::waitForDidFlush): Deleted.
(WebKit::RemoteRenderingBackendProxy::didFlush): Deleted.
* Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h:
* Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.messages.in:

Canonical link: https://commits.webkit.org/263572@main

Identifier: 263769.56@safari-7616.1.14.11-branch

Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.cpp

@@ -630,10 +630,16 @@ void RemoteDisplayListRecorder::applyDeviceScaleFactor(float scaleFactor)
     handleItem(DisplayList::ApplyDeviceScaleFactor(scaleFactor));
 }
 
-void RemoteDisplayListRecorder::flushContext(DisplayListRecorderFlushIdentifier identifier)
+void RemoteDisplayListRecorder::flushContext(IPC::Semaphore&& semaphore)
 {
     m_imageBuffer->flushDrawingContext();
-    m_renderingBackend->didFlush(identifier);
+    semaphore.signal();
+}
+
+void RemoteDisplayListRecorder::flushContextSync(CompletionHandler<void()>&& completionHandler)
+{
+    m_imageBuffer->flushDrawingContext();
+    completionHandler();
 }
 
 } // namespace WebKit

Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.h

@@ -132,7 +132,8 @@ class RemoteDisplayListRecorder : public IPC::StreamMessageReceiver, public CanM
     void applyFillPattern();
 #endif
     void applyDeviceScaleFactor(float);
-    void flushContext(DisplayListRecorderFlushIdentifier);
+    void flushContext(IPC::Semaphore&&);
+    void flushContextSync(CompletionHandler<void()>&&);
 
 private:
     RemoteDisplayListRecorder(WebCore::ImageBuffer&, QualifiedRenderingResourceIdentifier, WebCore::ProcessIdentifier webProcessIdentifier, RemoteRenderingBackend&);

Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.messages.in

@@ -98,7 +98,8 @@ messages -> RemoteDisplayListRecorder NotRefCounted Stream {
     ApplyFillPattern()
 #endif
     ApplyDeviceScaleFactor(float scaleFactor)
-    FlushContext(WebKit::DisplayListRecorderFlushIdentifier identifier)
+    FlushContext(IPC::Semaphore semaphore) NotStreamEncodable
+    FlushContextSync() -> () Synchronous
 
 #if PLATFORM(COCOA) && ENABLE(VIDEO)
     PaintVideoFrame(struct WebKit::SharedVideoFrame frame, WebCore::FloatRect rect, bool shouldDiscardAlpha) NotStreamEncodable

Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp

@@ -179,11 +179,6 @@ void RemoteRenderingBackend::didCreateImageBufferBackend(ImageBufferBackendHandl
     send(Messages::RemoteRenderingBackendProxy::DidCreateImageBufferBackend(WTFMove(handle), renderingResourceIdentifier.object()), m_renderingBackendIdentifier);
 }
 
-void RemoteRenderingBackend::didFlush(DisplayListRecorderFlushIdentifier flushIdentifier)
-{
-    send(Messages::RemoteRenderingBackendProxy::DidFlush(flushIdentifier));
-}
-
 void RemoteRenderingBackend::createImageBuffer(const FloatSize& logicalSize, RenderingMode renderingMode, RenderingPurpose purpose, float resolutionScale, const DestinationColorSpace& colorSpace, PixelFormat pixelFormat, RenderingResourceIdentifier imageBufferResourceIdentifier)
 {
     // Immediately turn the RenderingResourceIdentifier (which is error-prone) to a QualifiedRenderingResourceIdentifier,

Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h

@@ -97,7 +97,6 @@ class RemoteRenderingBackend : private IPC::MessageSender, public IPC::StreamMes
 
     // Messages to be sent.
     void didCreateImageBufferBackend(ImageBufferBackendHandle, QualifiedRenderingResourceIdentifier, RemoteDisplayListRecorder&);
-    void didFlush(DisplayListRecorderFlushIdentifier);
 
     // Runs Function in RemoteRenderingBackend task queue.
     void dispatch(Function<void()>&&);

Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.cpp

@@ -76,6 +76,16 @@ ALWAYS_INLINE void RemoteDisplayListRecorderProxy::send(T&& message)
     m_renderingBackend->streamConnection().send(WTFMove(message), m_destinationBufferIdentifier, defaultSendTimeout);
 }
 
+template<typename T>
+ALWAYS_INLINE void RemoteDisplayListRecorderProxy::sendSync(T&& message)
+{
+    if (UNLIKELY(!(m_renderingBackend && m_imageBuffer)))
+        return;
+
+    m_imageBuffer->backingStoreWillChange();
+    m_renderingBackend->streamConnection().sendSync(WTFMove(message), m_destinationBufferIdentifier, defaultSendTimeout);
+}
+
 RenderingMode RemoteDisplayListRecorderProxy::renderingMode() const
 {
     return m_imageBuffer ? m_imageBuffer->renderingMode() : RenderingMode::Unaccelerated;
@@ -524,7 +534,7 @@ bool RemoteDisplayListRecorderProxy::recordResourceUse(Filter& filter)
 
 void RemoteDisplayListRecorderProxy::flushContext(const IPC::Semaphore& semaphore)
 {
-    send(Messages::RemoteDisplayListRecorder::FlushContext(semaphore));
+   send(Messages::RemoteDisplayListRecorder::FlushContext(semaphore));
 }
 
 void RemoteDisplayListRecorderProxy::flushContextSync()

Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.h

@@ -50,11 +50,14 @@ class RemoteDisplayListRecorderProxy : public WebCore::DisplayList::Recorder {
 
     void convertToLuminanceMask() final;
     void transformToColorSpace(const WebCore::DestinationColorSpace&) final;
-    void flushContext(DisplayListRecorderFlushIdentifier);
+    void flushContext(const IPC::Semaphore&);
+    void flushContextSync();
     void disconnect();
 
+    static inline constexpr Seconds defaultSendTimeout = 3_s;
 private:
     template<typename T> void send(T&& message);
+    template<typename T> void sendSync(T&& message);
 
     friend class WebCore::DrawGlyphsRecorder;
 
@@ -147,7 +150,6 @@ class RemoteDisplayListRecorderProxy : public WebCore::DisplayList::Recorder {
     RefPtr<WebCore::ImageBuffer> createAlignedImageBuffer(const WebCore::FloatSize&, const WebCore::DestinationColorSpace&, std::optional<WebCore::RenderingMethod>) const final;
     RefPtr<WebCore::ImageBuffer> createAlignedImageBuffer(const WebCore::FloatRect&, const WebCore::DestinationColorSpace&, std::optional<WebCore::RenderingMethod>) const final;
 
-    static inline constexpr Seconds defaultSendTimeout = 3_s;
     WebCore::RenderingResourceIdentifier m_destinationBufferIdentifier;
     WeakPtr<RemoteImageBufferProxy> m_imageBuffer;
     WeakPtr<RemoteRenderingBackendProxy> m_renderingBackend;

Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.cpp

@@ -54,7 +54,7 @@ class RemoteImageBufferProxyFlusher final : public ThreadSafeImageBufferFlusher
 
     void flush() final
     {
-        m_flushState->waitForDidFlushOnSecondaryThread(m_targetFlushIdentifier);
+        m_flushState->waitForDidFlush(m_targetFlushIdentifier, Seconds::infinity());
     }
 
 private:
@@ -130,29 +130,6 @@ void RemoteImageBufferProxy::didCreateImageBufferBackend(ImageBufferBackendHandl
         m_backend = AcceleratedImageBufferRemoteBackend::create(parameters(), WTFMove(handle));
 }
 
-void RemoteImageBufferProxy::waitForDidFlushWithTimeout()
-{
-    if (!m_remoteRenderingBackendProxy)
-        return;
-
-    // Wait for our DisplayList to be flushed but do not hang.
-    static constexpr unsigned maximumNumberOfTimeouts = 3;
-    unsigned numberOfTimeouts = 0;
-#if !LOG_DISABLED
-    auto startTime = MonotonicTime::now();
-#endif
-    LOG_WITH_STREAM(SharedDisplayLists, stream << "RemoteImageBufferProxy " << m_renderingResourceIdentifier << " waitForDidFlushWithTimeout: waiting for flush {" << m_sentFlushIdentifier);
-    while (numberOfTimeouts < maximumNumberOfTimeouts && hasPendingFlush()) {
-        if (!m_remoteRenderingBackendProxy->waitForDidFlush())
-            ++numberOfTimeouts;
-    }
-
-    LOG_WITH_STREAM(SharedDisplayLists, stream << "RemoteImageBufferProxy " << m_renderingResourceIdentifier << " waitForDidFlushWithTimeout: done waiting " << (MonotonicTime::now() - startTime).milliseconds() << "ms; " << numberOfTimeouts << " timeout(s)");
-
-    if (UNLIKELY(numberOfTimeouts >= maximumNumberOfTimeouts))
-        RELEASE_LOG_FAULT(SharedDisplayLists, "Exceeded timeout while waiting for flush in remote rendering backend: %" PRIu64 ".", m_remoteRenderingBackendProxy->renderingBackendIdentifier().toUInt64());
-}
-
 ImageBufferBackend* RemoteImageBufferProxy::ensureBackendCreated() const
 {
     if (!m_remoteRenderingBackendProxy)
@@ -300,10 +277,14 @@ void RemoteImageBufferProxy::flushDrawingContext()
 
     TraceScope tracingScope(FlushRemoteImageBufferStart, FlushRemoteImageBufferEnd);
 
-    bool shouldWait = flushDrawingContextAsync();
-    LOG_WITH_STREAM(SharedDisplayLists, stream << "RemoteImageBufferProxy " << m_renderingResourceIdentifier << " flushDrawingContext: shouldWait " << shouldWait);
-    if (shouldWait)
-        waitForDidFlushWithTimeout();
+    if (!m_needsFlush) {
+        if (hasPendingFlush())
+            m_flushState->waitForDidFlush(m_sentFlushIdentifier, RemoteDisplayListRecorderProxy::defaultSendTimeout);
+        return;
+    }
+
+    m_remoteDisplayList.flushContextSync();
+    m_needsFlush = false;
 }
 
 bool RemoteImageBufferProxy::flushDrawingContextAsync()
@@ -316,8 +297,9 @@ bool RemoteImageBufferProxy::flushDrawingContextAsync()
 
     m_sentFlushIdentifier = DisplayListRecorderFlushIdentifier::generate();
     LOG_WITH_STREAM(SharedDisplayLists, stream << "RemoteImageBufferProxy " << m_renderingResourceIdentifier << " flushDrawingContextAsync - flush " << m_sentFlushIdentifier);
-    m_remoteDisplayList.flushContext(m_sentFlushIdentifier);
-    m_remoteRenderingBackendProxy->addPendingFlush(m_flushState, m_sentFlushIdentifier);
+    IPC::Semaphore flushSemaphore;
+    m_remoteDisplayList.flushContext(flushSemaphore);
+    m_remoteRenderingBackendProxy->addPendingFlush(m_flushState, WTFMove(flushSemaphore), m_sentFlushIdentifier);
     m_needsFlush = false;
     return true;
 }
@@ -340,12 +322,12 @@ void RemoteImageBufferProxy::prepareForBackingStoreChange()
         backend->ensureNativeImagesHaveCopiedBackingStore();
 }
 
-void RemoteImageBufferProxyFlushState::waitForDidFlushOnSecondaryThread(DisplayListRecorderFlushIdentifier targetFlushIdentifier)
+void RemoteImageBufferProxyFlushState::waitForDidFlush(DisplayListRecorderFlushIdentifier targetFlushIdentifier, Seconds timeout)
 {
     Locker locker { m_lock };
     if (m_identifier >= targetFlushIdentifier)
         return;
-    m_condition.wait(m_lock, [&] {
+    m_condition.waitFor(m_lock, timeout, [&] {
         assertIsHeld(m_lock);
         return m_identifier >= targetFlushIdentifier;
     });

Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h

@@ -76,8 +76,6 @@ class RemoteImageBufferProxy : public WebCore::ImageBuffer {
 
     bool hasPendingFlush() const;
 
-    void waitForDidFlushWithTimeout();
-
     RefPtr<WebCore::NativeImage> copyNativeImage(WebCore::BackingStoreCopy = WebCore::CopyBackingStore) const final;
     RefPtr<WebCore::NativeImage> copyNativeImageForDrawing(WebCore::GraphicsContext&) const final;
     RefPtr<WebCore::NativeImage> sinkIntoNativeImage() final;
@@ -119,7 +117,7 @@ class RemoteImageBufferProxyFlushState : public ThreadSafeRefCounted<RemoteImage
     WTF_MAKE_FAST_ALLOCATED;
 public:
     RemoteImageBufferProxyFlushState() = default;
-    void waitForDidFlushOnSecondaryThread(DisplayListRecorderFlushIdentifier);
+    void waitForDidFlush(DisplayListRecorderFlushIdentifier, Seconds timeout);
     void markCompletedFlush(DisplayListRecorderFlushIdentifier);
     void cancel();
     DisplayListRecorderFlushIdentifier identifierForCompletedFlush() const;

Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp

@@ -60,6 +60,7 @@ std::unique_ptr<RemoteRenderingBackendProxy> RemoteRenderingBackendProxy::create
 RemoteRenderingBackendProxy::RemoteRenderingBackendProxy(const RemoteRenderingBackendCreationParameters& parameters, SerialFunctionDispatcher& dispatcher)
     : m_parameters(parameters)
     , m_dispatcher(dispatcher)
+    , m_flushWorkQueue(WorkQueue::create("RemoteRenderingBackendProxy flush queue"_s))
 {
 }
 
@@ -113,9 +114,6 @@ void RemoteRenderingBackendProxy::disconnectGPUProcess()
     m_didRenderingUpdateID = { };
     m_streamConnection->invalidate();
     m_streamConnection = nullptr;
-    auto pendingFlushes = std::exchange(m_pendingFlushes, { });
-    for (auto& flushState : pendingFlushes.values())
-        flushState->cancel();
 }
 
 RemoteRenderingBackendProxy::DidReceiveBackendCreationResult RemoteRenderingBackendProxy::waitForDidCreateImageBufferBackend()
@@ -125,11 +123,6 @@ RemoteRenderingBackendProxy::DidReceiveBackendCreationResult RemoteRenderingBack
     return DidReceiveBackendCreationResult::ReceivedAnyResponse;
 }
 
-bool RemoteRenderingBackendProxy::waitForDidFlush()
-{
-    return streamConnection().waitForAndDispatchImmediately<Messages::RemoteRenderingBackendProxy::DidFlush>(renderingBackendIdentifier(), 1_s, IPC::WaitForOption::InterruptWaitingIfSyncMessageArrives);
-}
-
 void RemoteRenderingBackendProxy::createRemoteImageBuffer(ImageBuffer& imageBuffer)
 {
     send(Messages::RemoteRenderingBackend::CreateImageBuffer(imageBuffer.logicalSize(), imageBuffer.renderingMode(), imageBuffer.renderingPurpose(), imageBuffer.resolutionScale(), imageBuffer.colorSpace(), imageBuffer.pixelFormat(), imageBuffer.renderingResourceIdentifier()));
@@ -450,13 +443,6 @@ void RemoteRenderingBackendProxy::didCreateImageBufferBackend(ImageBufferBackend
     imageBuffer->didCreateImageBufferBackend(WTFMove(handle));
 }
 
-void RemoteRenderingBackendProxy::didFlush(DisplayListRecorderFlushIdentifier flushIdentifier)
-{
-    auto flush = m_pendingFlushes.take(flushIdentifier);
-    ASSERT(flush);
-    flush->markCompletedFlush(flushIdentifier);
-}
-
 void RemoteRenderingBackendProxy::didFinalizeRenderingUpdate(RenderingUpdateID didRenderingUpdateID)
 {
     ASSERT(didRenderingUpdateID <= m_renderingUpdateID);
@@ -491,10 +477,14 @@ void RemoteRenderingBackendProxy::didInitialize(IPC::Semaphore&& wakeUp, IPC::Se
     m_streamConnection->setSemaphores(WTFMove(wakeUp), WTFMove(clientWait));
 }
 
-void RemoteRenderingBackendProxy::addPendingFlush(RemoteImageBufferProxyFlushState& flushState, DisplayListRecorderFlushIdentifier identifier)
+void RemoteRenderingBackendProxy::addPendingFlush(RemoteImageBufferProxyFlushState& flushState, IPC::Semaphore&& semaphore, DisplayListRecorderFlushIdentifier identifier)
 {
-    auto result = m_pendingFlushes.add(identifier, flushState);
-    ASSERT_UNUSED(result, result.isNewEntry);
+    m_flushWorkQueue->dispatch([flushState = Ref { flushState }, semaphore = WTFMove(semaphore), identifier] () mutable {
+        if (semaphore.wait())
+            flushState->markCompletedFlush(identifier);
+        else
+            flushState->cancel();
+    });
 }
 
 bool RemoteRenderingBackendProxy::isCached(const ImageBuffer& imageBuffer) const

Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h

@@ -27,6 +27,7 @@
 
 #if ENABLE(GPU_PROCESS)
 
+#include "DisplayListRecorderFlushIdentifier.h"
 #include "GPUProcessConnection.h"
 #include "IPCSemaphore.h"
 #include "ImageBufferBackendHandle.h"
@@ -48,6 +49,7 @@
 #include <wtf/HashMap.h>
 #include <wtf/Span.h>
 #include <wtf/WeakPtr.h>
+#include <wtf/WorkQueue.h>
 
 namespace WebCore {
 
@@ -138,7 +140,6 @@ class RemoteRenderingBackendProxy
         TimeoutOrIPCFailure
     };
     DidReceiveBackendCreationResult waitForDidCreateImageBufferBackend();
-    bool waitForDidFlush();
 
     RenderingBackendIdentifier renderingBackendIdentifier() const;
 
@@ -159,7 +160,7 @@ class RemoteRenderingBackendProxy
 
     SerialFunctionDispatcher& dispatcher() { return m_dispatcher; }
 
-    void addPendingFlush(RemoteImageBufferProxyFlushState&, DisplayListRecorderFlushIdentifier);
+    void addPendingFlush(RemoteImageBufferProxyFlushState&, IPC::Semaphore&&, DisplayListRecorderFlushIdentifier);
 
 private:
     explicit RemoteRenderingBackendProxy(const RemoteRenderingBackendCreationParameters&, SerialFunctionDispatcher&);
@@ -190,7 +191,6 @@ class RemoteRenderingBackendProxy
 
     // Messages to be received.
     void didCreateImageBufferBackend(ImageBufferBackendHandle&&, WebCore::RenderingResourceIdentifier);
-    void didFlush(DisplayListRecorderFlushIdentifier);
     void didFinalizeRenderingUpdate(RenderingUpdateID didRenderingUpdateID);
     void didMarkLayersAsVolatile(MarkSurfacesAsVolatileRequestIdentifier, const Vector<WebCore::RenderingResourceIdentifier>& markedVolatileBufferIdentifiers, bool didMarkAllLayerAsVolatile);
 
@@ -205,7 +205,7 @@ class RemoteRenderingBackendProxy
 
     RenderingUpdateID m_renderingUpdateID;
     RenderingUpdateID m_didRenderingUpdateID;
-    HashMap<DisplayListRecorderFlushIdentifier, Ref<RemoteImageBufferProxyFlushState>> m_pendingFlushes;
+    Ref<WorkQueue> m_flushWorkQueue;
 };
 
 } // namespace WebKit

Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.messages.in

@@ -24,7 +24,6 @@
 
 messages -> RemoteRenderingBackendProxy NotRefCounted {
     DidCreateImageBufferBackend(WebKit::ImageBufferBackendHandle handle, WebCore::RenderingResourceIdentifier renderingResourceIdentifier)
-    DidFlush(WebKit::DisplayListRecorderFlushIdentifier flushIdentifier)
     DidFinalizeRenderingUpdate(WebKit::RenderingUpdateID didRenderingUpdateID)
     DidInitialize(IPC::Semaphore wakeUpSemaphore, IPC::Semaphore clientWaitSemaphore)
     DidMarkLayersAsVolatile(WebKit::MarkSurfacesAsVolatileRequestIdentifier requestIdentifier, Vector<WebCore::RenderingResourceIdentifier> markedVolatileBufferIdentifiers, bool didMarkAllLayersAsVolatile)