Attachment 434067 Details for Bug 228222 – Patch for reviewing

[patch] Patch for reviewing

patch.diff (text/plain), 32.03 KB, created by Myles C. Maxfield on 2021-07-23 00:13:28 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Myles C. Maxfield

Created: 2021-07-23 00:13:28 PDT

Size: 32.03 KB

patch

obsolete

>commit ee8a43d81e9e672dc7444e6642e2558af31623e8
>Author: Myles C. Maxfield <mmaxfield@apple.com>
>Date:   Fri Jul 23 00:12:13 2021 -0700
>
>    [GPU Process] Add resource use counter infrastructure to RemoteResourceCache
>    https://bugs.webkit.org/show_bug.cgi?id=228222
>    
>    Reviewed by NOBODY (OOPS!).
>    
>    Source/WebCore:
>    
>    This is part 2 of https://bugs.webkit.org/show_bug.cgi?id=228216.
>    
>    Using DisplayList::Replayer::Delegate to track how many times a resource is used in a display list is a natural fit.
>    However, if the GPU process determines that a resource has hit its use limit and is ready to be destroyed, it can't
>    actually destroy it immediately, because the delegate's callback is called before actually executing the display
>    list item (which needs to see the resource). And it has to execute before executing the display list item because
>    the delegate actually implements the display list item for things like getImageData. So, this patch adds another
>    method to the delegate: didCompleteReplaying(), which is called at the end of the replay. The GPU process will
>    remember which resources were used during the replay, and increment the relevant counters, but only once the replay
>    has finished can the GPU process actually delete these resources. That occurs in this new delegate method.
>    
>    No new tests because there is no behavior change yet. This patch is just adding infrastructure. For now, all these
>    use counts sent by the web process are 0 - I'll implement that part in 3 follow-up patches.
>    
>    * platform/graphics/displaylists/DisplayListReplayer.cpp:
>    (WebCore::DisplayList::Replayer::replay):
>    * platform/graphics/displaylists/DisplayListReplayer.h:
>    (WebCore::DisplayList::Replayer::Delegate::didCompleteReplaying):
>    
>    Source/WebKit:
>    
>    This patch adds the infrastructure for associating a counter with each resource in the RemoteResourceCache.
>    As the web process uses a resource during display list recording, it increments a counter, and as the GPU
>    process uses a resource, it increments a counter in the RemoteResourceCache. When the web process is done
>    with a resource, it sends a release message to the GPU process which crucially includes the web process's
>    counter. The GPU process has to then wait to actually delete the resource until its own use counter matches
>    the one it received from the web process. This patch implements this deferred deletion infrastructure
>    in the GPU process.
>    
>    * GPUProcess/graphics/RemoteRenderingBackend.cpp:
>    (WebKit::RemoteRenderingBackend::ReplayerDelegate::didCompleteReplaying): See the WebCore ChangeLog for an
>    explanation of this.
>    (WebKit::RemoteRenderingBackend::releaseRemoteResource): Pass the use count to the RemoteResourceCache.
>    * GPUProcess/graphics/RemoteRenderingBackend.h: Add the use count field.
>    * GPUProcess/graphics/RemoteRenderingBackend.messages.in: Ditto.
>    * GPUProcess/graphics/RemoteResourceCache.cpp:
>    (WebKit::RemoteResourceCache::cacheImageBuffer): It's actually okay if there's already an resource cached.
>    That just means that the web process started using a resource again before the GPU process consumed all
>    commands that used it the first time. All we need to do is increment the open count.
>    (WebKit::RemoteResourceCache::cacheNativeImage): Ditto.
>    (WebKit::RemoteResourceCache::cacheFont): Ditto.
>    (WebKit::RemoteResourceCache::incrementOpenCount):
>    (WebKit::RemoteResourceCache::maybeRemoveResource): This is called whenever it's possible for us to be in
>    situation where we should be removing a resource (aka whenever the use count increments). It checks to see
>    if we can remove the resource, and if it can, does so.
>    (WebKit::RemoteResourceCache::recordResourceUse): The callback that runs for each resource use during
>    replaying. It can't actually delete any resources, so instead it just records which resources had their
>    use counts incremented, and waits for prune() to actually call maybeRemoveResource().
>    (WebKit::RemoteResourceCache::prune): After the replay is complete, call maybeRemoveResource() on all the
>    resources which had their use counters touched.
>    (WebKit::RemoteResourceCache::releaseRemoteResource): Put the relevant resource into the state where we
>    are waiting for the use counters to match before we can delete the resource. Also, call maybeRemoveResource()
>    so it can be deleted immediately if the counters already match.
>    * GPUProcess/graphics/RemoteResourceCache.h: Add a new data structure to hold the use counters. There's
>    a big comment in here that describes what these fields mean.
>    * WebProcess/GPU/graphics/RemoteImageBufferProxy.h:
>    (WebKit::RemoteImageBufferProxy::~RemoteImageBufferProxy): Send a dummy use count for now. This will be
>    implemented in a follow-up patch.
>    * WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:
>    (WebKit::RemoteRenderingBackendProxy::releaseRemoteResource): Add the useCount parameter.
>    * WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h: Ditto.
>    * WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:
>    (WebKit::RemoteResourceCacheProxy::releaseNativeImage): Send a dummy use count for now. This will be
>    implemented in a follow-up patch.
>    (WebKit::RemoteResourceCacheProxy::clearFontMap): Ditto.
>    (WebKit::RemoteResourceCacheProxy::finalizeRenderingUpdateForFonts): Ditto.
>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 8e5cba30ceef..49b73a738c50 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,29 @@
>+2021-07-22  Myles C. Maxfield  <mmaxfield@apple.com>
>+
>+        [GPU Process] Add resource use counter infrastructure to RemoteResourceCache
>+        https://bugs.webkit.org/show_bug.cgi?id=228222
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This is part 2 of https://bugs.webkit.org/show_bug.cgi?id=228216.
>+
>+        Using DisplayList::Replayer::Delegate to track how many times a resource is used in a display list is a natural fit.
>+        However, if the GPU process determines that a resource has hit its use limit and is ready to be destroyed, it can't
>+        actually destroy it immediately, because the delegate's callback is called before actually executing the display
>+        list item (which needs to see the resource). And it has to execute before executing the display list item because
>+        the delegate actually implements the display list item for things like getImageData. So, this patch adds another
>+        method to the delegate: didCompleteReplaying(), which is called at the end of the replay. The GPU process will
>+        remember which resources were used during the replay, and increment the relevant counters, but only once the replay
>+        has finished can the GPU process actually delete these resources. That occurs in this new delegate method.
>+
>+        No new tests because there is no behavior change yet. This patch is just adding infrastructure. For now, all these
>+        use counts sent by the web process are 0 - I'll implement that part in 3 follow-up patches.
>+
>+        * platform/graphics/displaylists/DisplayListReplayer.cpp:
>+        (WebCore::DisplayList::Replayer::replay):
>+        * platform/graphics/displaylists/DisplayListReplayer.h:
>+        (WebCore::DisplayList::Replayer::Delegate::didCompleteReplaying):
>+
> 2021-07-22  Said Abou-Hallawa  <said@apple.com>
> 
>         Unreviewed, reverting 280130.
>diff --git a/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp b/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp
>index f370b4118d02..3561da0bd122 100644
>--- a/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp
>+++ b/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp
>@@ -232,6 +232,9 @@ ReplayResult Replayer::replay(const FloatRect& initialClip, bool trackReplayList
>         }
>     }
> 
>+    if (m_delegate)
>+        m_delegate->didCompleteReplaying();
>+
>     result.trackedDisplayList = WTFMove(replayList);
>     return result;
> }
>diff --git a/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h b/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h
>index 16627a0206e3..57939ee977ed 100644
>--- a/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h
>+++ b/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h
>@@ -68,6 +68,7 @@ public:
>         virtual bool apply(ItemHandle, GraphicsContext&) { return false; }
>         virtual void didCreateMaskImageBuffer(WebCore::ImageBuffer&) { }
>         virtual void didResetMaskImageBuffer() { }
>+        virtual void didCompleteReplaying() { }
>     };
>     
> private:
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index fba043306643..0f37f4aa2ea4 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,42 +1,55 @@
> 2021-07-22  Myles C. Maxfield  <mmaxfield@apple.com>
> 
>-        [GPU Process] Migrate the DisplayList::Replayer::Delegate from RemoteImageBuffer to RemoteRenderingBackend
>-        https://bugs.webkit.org/show_bug.cgi?id=228219
>+        [GPU Process] Add resource use counter infrastructure to RemoteResourceCache
>+        https://bugs.webkit.org/show_bug.cgi?id=228222
> 
>         Reviewed by NOBODY (OOPS!).
> 
>-        This is part 1 of https://bugs.webkit.org/show_bug.cgi?id=228216.
>+        This patch adds the infrastructure for associating a counter with each resource in the RemoteResourceCache.
>+        As the web process uses a resource during display list recording, it increments a counter, and as the GPU
>+        process uses a resource, it increments a counter in the RemoteResourceCache. When the web process is done
>+        with a resource, it sends a release message to the GPU process which crucially includes the web process's
>+        counter. The GPU process has to then wait to actually delete the resource until its own use counter matches
>+        the one it received from the web process. This patch implements this deferred deletion infrastructure
>+        in the GPU process.
> 
>-        For this task, we need to count resource uses:
>-        1. As the recorder in the web process records DisplayList items which reference resources, those uses need to increment a counter.
>-        2. As the replayer in the GPU process replays DisplayList items which reference resources, those uses need to increment a parallel counter.
>-
>-        The most natural place for the counters to live is in RemoteResourceCacheProxy and RemoteResourceCache, respectively. These two classes
>-        are accessible from the RemoteRenderingBackendProxy and RemoteRenderingBackend, respectively. The most natural interface between the
>-        DisplayList classes like DisplayList::Recorder and DisplayList::Replayer and the higher level WebKit classe is to use their delegate
>-        interfaces.
>-
>-        For 1 above, RemoteImageBufferProxy has access to the RemoteRenderingBackendProxy and is already a DisplayList::Recorder::Delegate, so
>-        there's no problem there. However, for 2, messages are delivered first to the RemoteRenderingBackend, which is the place that has access to
>-        the RemoteResourceCache. Making the RemoteRenderingBackend have the DisplayList::Replayer::Delegate would be a natural place to either
>-        A) call the necessary function in the RemoteImageBuffer, or B) interact with the RemoteResourceCache to increment the necessary counter.
>-
>-        Indeed, this makes a lot of sense because, for 2 of the 3 delegate methods, RemoteImageBuffer just immediately turns around and forwards
>-        the call to the RemoteRenderingBackend anyway. So, migrating this interface to RemoteRenderingBackend actually ends up making it a bit
>-        simpler.
>-
>-        No new tests because there is no behavior change.
>-
>-        * GPUProcess/graphics/RemoteImageBuffer.h:
>-        (WebKit::RemoteImageBuffer::apply):
>-        (): Deleted.
>         * GPUProcess/graphics/RemoteRenderingBackend.cpp:
>-        (WebKit::RemoteRenderingBackend::ReplayerDelegate::ReplayerDelegate):
>-        (WebKit::RemoteRenderingBackend::ReplayerDelegate::apply):
>-        (WebKit::RemoteRenderingBackend::ReplayerDelegate::didCreateMaskImageBuffer):
>-        (WebKit::RemoteRenderingBackend::ReplayerDelegate::didResetMaskImageBuffer):
>-        (WebKit::RemoteRenderingBackend::submit):
>-        * GPUProcess/graphics/RemoteRenderingBackend.h:
>+        (WebKit::RemoteRenderingBackend::ReplayerDelegate::didCompleteReplaying): See the WebCore ChangeLog for an
>+        explanation of this.
>+        (WebKit::RemoteRenderingBackend::releaseRemoteResource): Pass the use count to the RemoteResourceCache.
>+        * GPUProcess/graphics/RemoteRenderingBackend.h: Add the use count field.
>+        * GPUProcess/graphics/RemoteRenderingBackend.messages.in: Ditto.
>+        * GPUProcess/graphics/RemoteResourceCache.cpp:
>+        (WebKit::RemoteResourceCache::cacheImageBuffer): It's actually okay if there's already an resource cached.
>+        That just means that the web process started using a resource again before the GPU process consumed all
>+        commands that used it the first time. All we need to do is increment the open count.
>+        (WebKit::RemoteResourceCache::cacheNativeImage): Ditto.
>+        (WebKit::RemoteResourceCache::cacheFont): Ditto.
>+        (WebKit::RemoteResourceCache::incrementOpenCount):
>+        (WebKit::RemoteResourceCache::maybeRemoveResource): This is called whenever it's possible for us to be in
>+        situation where we should be removing a resource (aka whenever the use count increments). It checks to see
>+        if we can remove the resource, and if it can, does so.
>+        (WebKit::RemoteResourceCache::recordResourceUse): The callback that runs for each resource use during
>+        replaying. It can't actually delete any resources, so instead it just records which resources had their
>+        use counts incremented, and waits for prune() to actually call maybeRemoveResource().
>+        (WebKit::RemoteResourceCache::prune): After the replay is complete, call maybeRemoveResource() on all the
>+        resources which had their use counters touched.
>+        (WebKit::RemoteResourceCache::releaseRemoteResource): Put the relevant resource into the state where we
>+        are waiting for the use counters to match before we can delete the resource. Also, call maybeRemoveResource()
>+        so it can be deleted immediately if the counters already match.
>+        * GPUProcess/graphics/RemoteResourceCache.h: Add a new data structure to hold the use counters. There's
>+        a big comment in here that describes what these fields mean.
>+        * WebProcess/GPU/graphics/RemoteImageBufferProxy.h:
>+        (WebKit::RemoteImageBufferProxy::~RemoteImageBufferProxy): Send a dummy use count for now. This will be
>+        implemented in a follow-up patch.
>+        * WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:
>+        (WebKit::RemoteRenderingBackendProxy::releaseRemoteResource): Add the useCount parameter.
>+        * WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h: Ditto.
>+        * WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:
>+        (WebKit::RemoteResourceCacheProxy::releaseNativeImage): Send a dummy use count for now. This will be
>+        implemented in a follow-up patch.
>+        (WebKit::RemoteResourceCacheProxy::clearFontMap): Ditto.
>+        (WebKit::RemoteResourceCacheProxy::finalizeRenderingUpdateForFonts): Ditto.
> 
> 2021-07-22  Devin Rousso  <drousso@apple.com>
> 
>diff --git a/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp b/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp
>index ea8286da0c9d..c106f42cc4f3 100644
>--- a/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp
>+++ b/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp
>@@ -214,6 +214,12 @@ void RemoteRenderingBackend::ReplayerDelegate::didResetMaskImageBuffer()
>     m_remoteRenderingBackend.didResetMaskImageBuffer();
> }
> 
>+void RemoteRenderingBackend::ReplayerDelegate::didCompleteReplaying()
>+{
>+    auto messagesAreInOrder = m_remoteRenderingBackend.remoteResourceCache().prune();
>+    MESSAGE_CHECK(messagesAreInOrder, "Web process tried to release a resource that wasn't cached");
>+}
>+
> DisplayList::ReplayResult RemoteRenderingBackend::submit(const DisplayList::DisplayList& displayList, ImageBuffer& destination)
> {
>     if (displayList.isEmpty())
>@@ -509,10 +515,11 @@ void RemoteRenderingBackend::deleteAllFonts()
>     m_remoteResourceCache.deleteAllFonts();
> }
> 
>-void RemoteRenderingBackend::releaseRemoteResource(RenderingResourceIdentifier renderingResourceIdentifier)
>+void RemoteRenderingBackend::releaseRemoteResource(RenderingResourceIdentifier renderingResourceIdentifier, uint64_t useCount)
> {
>     ASSERT(!RunLoop::isMain());
>-    m_remoteResourceCache.releaseRemoteResource(renderingResourceIdentifier);
>+    auto success = m_remoteResourceCache.releaseRemoteResource(renderingResourceIdentifier, useCount);
>+    MESSAGE_CHECK(success, "Resource is being released before being cached.");
>     updateRenderingResourceRequest();
> }
> 
>diff --git a/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h b/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h
>index f7bf6bc3b073..872629f0253b 100644
>--- a/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h
>+++ b/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h
>@@ -144,7 +144,7 @@ private:
>     void cacheNativeImage(const ShareableBitmap::Handle&, WebCore::RenderingResourceIdentifier);
>     void cacheFont(Ref<WebCore::Font>&&);
>     void deleteAllFonts();
>-    void releaseRemoteResource(WebCore::RenderingResourceIdentifier);
>+    void releaseRemoteResource(WebCore::RenderingResourceIdentifier, uint64_t useCount);
>     void didCreateSharedDisplayListHandle(WebCore::DisplayList::ItemBufferIdentifier, const SharedMemory::IPCHandle&, WebCore::RenderingResourceIdentifier destinationBufferIdentifier);
> 
>     class ReplayerDelegate : public WebCore::DisplayList::Replayer::Delegate {
>@@ -155,6 +155,7 @@ private:
>         bool apply(WebCore::DisplayList::ItemHandle, WebCore::GraphicsContext&) final;
>         void didCreateMaskImageBuffer(WebCore::ImageBuffer&) final;
>         void didResetMaskImageBuffer() final;
>+        void didCompleteReplaying() final;
> 
>         WebCore::ImageBuffer& m_destination;
>         RemoteRenderingBackend& m_remoteRenderingBackend;
>diff --git a/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in b/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in
>index 4b3fe9a0262e..ff62af511f3e 100644
>--- a/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in
>+++ b/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in
>@@ -36,7 +36,7 @@ messages -> RemoteRenderingBackend {
>     CacheFont(IPC::FontReference font)
>     DeleteAllFonts()
>     DidCreateSharedDisplayListHandle(WebCore::DisplayList::ItemBufferIdentifier identifier, WebKit::SharedMemory::IPCHandle handle, WebCore::RenderingResourceIdentifier destinationBufferIdentifier)
>-    ReleaseRemoteResource(WebCore::RenderingResourceIdentifier renderingResourceIdentifier)
>+    ReleaseRemoteResource(WebCore::RenderingResourceIdentifier renderingResourceIdentifier, uint64_t useCount)
> }
> 
> #endif // ENABLE(GPU_PROCESS)
>diff --git a/Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp b/Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp
>index 81dde0bffcf0..31a194f4a281 100644
>--- a/Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp
>+++ b/Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp
>@@ -33,8 +33,10 @@ using namespace WebCore;
> 
> void RemoteResourceCache::cacheImageBuffer(Ref<ImageBuffer>&& imageBuffer)
> {
>-    auto addResult = m_imageBuffers.add(imageBuffer->renderingResourceIdentifier(), WTFMove(imageBuffer));
>-    ASSERT_UNUSED(addResult, addResult.isNewEntry);
>+    auto renderingResourceIdentifier = imageBuffer->renderingResourceIdentifier();
>+    m_imageBuffers.add(renderingResourceIdentifier, WTFMove(imageBuffer));
>+
>+    incrementOpenCount(renderingResourceIdentifier);
> }
> 
> ImageBuffer* RemoteResourceCache::cachedImageBuffer(RenderingResourceIdentifier renderingResourceIdentifier)
>@@ -44,14 +46,27 @@ ImageBuffer* RemoteResourceCache::cachedImageBuffer(RenderingResourceIdentifier
> 
> void RemoteResourceCache::cacheNativeImage(Ref<NativeImage>&& image)
> {
>-    auto addResult = m_nativeImages.add(image->renderingResourceIdentifier(), WTFMove(image));
>-    ASSERT_UNUSED(addResult, addResult.isNewEntry);
>+    auto renderingResourceIdentifier = image->renderingResourceIdentifier();
>+    m_nativeImages.add(renderingResourceIdentifier, WTFMove(image));
>+
>+    incrementOpenCount(renderingResourceIdentifier);
> }
> 
> void RemoteResourceCache::cacheFont(Ref<Font>&& font)
> {
>-    auto addResult = m_fonts.add(font->renderingResourceIdentifier(), WTFMove(font));
>-    ASSERT_UNUSED(addResult, addResult.isNewEntry);
>+    auto renderingResourceIdentifier = font->renderingResourceIdentifier();
>+    m_fonts.add(renderingResourceIdentifier, WTFMove(font));
>+
>+    incrementOpenCount(renderingResourceIdentifier);
>+}
>+
>+void RemoteResourceCache::incrementOpenCount(WebCore::RenderingResourceIdentifier renderingResourceIdentifier)
>+{
>+    auto result = m_resourceUseCounters.ensure(renderingResourceIdentifier, [&] {
>+        return ResourceUseCounter { };
>+    });
>+    ++result.iterator->value.openCount;
>+    RELEASE_ASSERT(result.iterator->value.openCount);
> }
> 
> void RemoteResourceCache::deleteAllFonts()
>@@ -59,16 +74,72 @@ void RemoteResourceCache::deleteAllFonts()
>     m_fonts.clear();
> }
> 
>-void RemoteResourceCache::releaseRemoteResource(RenderingResourceIdentifier renderingResourceIdentifier)
>+bool RemoteResourceCache::maybeRemoveResource(WebCore::RenderingResourceIdentifier renderingResourceIdentifier, ResourceUseCountersMap::iterator iterator)
> {
>+    // It's actually okay if useCount > totalUseCount.
>+    // It just means that there's a cache message pending, and we've eagerly used a previously-cached resource.
>+    // Which is fine.
>+    // FIXME: Maybe we shouldn't actually remove the resource in this situation?
>+    // We may actually be able to get a head start on some display list items if they're ready for us and we have the resources they depend on.
>+
>+    if (iterator->value.openCount || iterator->value.useCount < iterator->value.totalUseCount)
>+        return true;
>+    iterator->value.useCount -= iterator->value.totalUseCount;
>+    iterator->value.totalUseCount = 0;
>+    if (!iterator->value.useCount)
>+        m_resourceUseCounters.remove(iterator);
>+
>     if (m_imageBuffers.remove(renderingResourceIdentifier))
>-        return;
>+        return true;
>     if (m_nativeImages.remove(renderingResourceIdentifier))
>-        return;
>+        return true;
>     if (m_fonts.remove(renderingResourceIdentifier))
>-        return;
>+        return true;
>+
>     // Caching the remote resource should have happened before releasing it.
>-    ASSERT_NOT_REACHED();
>+    return false;
>+}
>+
>+void RemoteResourceCache::recordResourceUse(WebCore::RenderingResourceIdentifier renderingResourceIdentifier)
>+{
>+    auto iterator = m_resourceUseCounters.find(renderingResourceIdentifier);
>+
>+    if (iterator == m_resourceUseCounters.end()) {
>+        // The display list items won the race against the cacheFont message.
>+        // When we receive the cacheFont message, we will retry the display list item.
>+        return;
>+    }
>+
>+    ResourceUseCounter& useCounter = iterator->value;
>+    ++useCounter.useCount;
>+
>+    m_resourcesForMaybeRemoving.add(renderingResourceIdentifier);
>+}
>+
>+bool RemoteResourceCache::prune()
>+{
>+    for (auto renderingResourceIdentifier : m_resourcesForMaybeRemoving) {
>+        auto iterator = m_resourceUseCounters.find(renderingResourceIdentifier);
>+        ASSERT(iterator != m_resourceUseCounters.end());
>+        auto messagesAreInOrder = maybeRemoveResource(renderingResourceIdentifier, iterator);
>+        if (!messagesAreInOrder)
>+            return false;
>+    }
>+    m_resourcesForMaybeRemoving.clear();
>+    return true;
>+}
>+
>+bool RemoteResourceCache::releaseRemoteResource(RenderingResourceIdentifier renderingResourceIdentifier, uint64_t useCount)
>+{
>+    auto iterator = m_resourceUseCounters.find(renderingResourceIdentifier);
>+    if (iterator == m_resourceUseCounters.end())
>+        return false;
>+    ResourceUseCounter& useCounter = iterator->value;
>+    if (!iterator->value.openCount)
>+        return false;
>+    --useCounter.openCount;
>+    useCounter.totalUseCount += useCount;
>+    return maybeRemoveResource(renderingResourceIdentifier, iterator);
> }
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h b/Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h
>index 2ced909f4811..ad274d73d161 100644
>--- a/Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h
>+++ b/Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h
>@@ -46,16 +46,45 @@ public:
>     void cacheNativeImage(Ref<WebCore::NativeImage>&&);
>     void cacheFont(Ref<WebCore::Font>&&);
>     void deleteAllFonts();
>-    void releaseRemoteResource(WebCore::RenderingResourceIdentifier);
>+    bool releaseRemoteResource(WebCore::RenderingResourceIdentifier, uint64_t useCount);
>+    void recordResourceUse(WebCore::RenderingResourceIdentifier);
>+    bool prune();
> 
>     const WebCore::ImageBufferHashMap& imageBuffers() const { return m_imageBuffers; }
>     const WebCore::NativeImageHashMap& nativeImages() const { return m_nativeImages; }
>     const WebCore::FontRenderingResourceMap& fonts() const { return m_fonts; }
> 
> private:
>+    // Because the cache/release messages are sent asynchronously from the display list items which
>+    // reference the resources, it's totally possible that we see a release message before we've
>+    // executed all the display list items which reference the resource. The web process tells us
>+    // how many display list items will reference this resource, and we defer deletion of the resource
>+    // until we execute that many display list items.
>+    //
>+    // It's actually a bit worse than this, though, because we may actually see a *new* cache message
>+    // during the time when deletion is deferred, so we need a second count of the number of cache
>+    // messages which don't have an accompanying release messages.
>+    //
>+    // We can only safely delete a resource when:
>+    // 1. All the cache messages have an accompanying release message, and
>+    // 2. We've processed as many display list items that reference a particular resource as the web
>+    // process has encoded.
>+    struct ResourceUseCounter {
>+        uint64_t openCount { 0 }; // The number of cacheFoo messages we've received without a corresponding release message
>+        uint64_t useCount { 0 }; // The number of rendering commands we've executed which reference this resource
>+        uint64_t totalUseCount { 0 }; // Once useCount hits this limit, we can free the resource
>+    };
>+    using ResourceUseCountersMap = HashMap<WebCore::RenderingResourceIdentifier, ResourceUseCounter>;
>+
>+    bool maybeRemoveResource(WebCore::RenderingResourceIdentifier, ResourceUseCountersMap::iterator);
>+    void incrementOpenCount(WebCore::RenderingResourceIdentifier);
>+
>     WebCore::ImageBufferHashMap m_imageBuffers;
>     WebCore::NativeImageHashMap m_nativeImages;
>     WebCore::FontRenderingResourceMap m_fonts;
>+
>+    ResourceUseCountersMap m_resourceUseCounters;
>+    HashSet<WebCore::RenderingResourceIdentifier> m_resourcesForMaybeRemoving;
> };
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h b/Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h
>index b448b37f97fe..d4cbc2c0c903 100644
>--- a/Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h
>+++ b/Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h
>@@ -72,7 +72,7 @@ public:
> 
>         flushDrawingContext();
>         m_remoteRenderingBackendProxy->remoteResourceCacheProxy().releaseImageBuffer(m_renderingResourceIdentifier);
>-        m_remoteRenderingBackendProxy->releaseRemoteResource(m_renderingResourceIdentifier);
>+        m_remoteRenderingBackendProxy->releaseRemoteResource(m_renderingResourceIdentifier, 0); // FIXME: Pass the real use count here.
>     }
> 
>     ImageBufferBackendHandle createImageBufferBackendHandle()
>diff --git a/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp b/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp
>index 09ce4c6ef6d6..e95f1ad3934f 100644
>--- a/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp
>+++ b/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp
>@@ -249,12 +249,12 @@ void RemoteRenderingBackendProxy::deleteAllFonts()
>     send(Messages::RemoteRenderingBackend::DeleteAllFonts(), renderingBackendIdentifier(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);
> }
> 
>-void RemoteRenderingBackendProxy::releaseRemoteResource(RenderingResourceIdentifier renderingResourceIdentifier)
>+void RemoteRenderingBackendProxy::releaseRemoteResource(RenderingResourceIdentifier renderingResourceIdentifier, uint64_t useCount)
> {
>     if (renderingResourceIdentifier == m_currentDestinationImageBufferIdentifier)
>         m_currentDestinationImageBufferIdentifier = std::nullopt;
> 
>-    send(Messages::RemoteRenderingBackend::ReleaseRemoteResource(renderingResourceIdentifier), renderingBackendIdentifier(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);
>+    send(Messages::RemoteRenderingBackend::ReleaseRemoteResource(renderingResourceIdentifier, useCount), renderingBackendIdentifier(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);
> }
> 
> void RemoteRenderingBackendProxy::didCreateImageBufferBackend(ImageBufferBackendHandle handle, RenderingResourceIdentifier renderingResourceIdentifier)
>diff --git a/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h b/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h
>index 63149670a359..b2ce3d8c28ab 100644
>--- a/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h
>+++ b/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h
>@@ -104,7 +104,7 @@ public:
>     void cacheNativeImage(const ShareableBitmap::Handle&, WebCore::RenderingResourceIdentifier);
>     void cacheFont(Ref<WebCore::Font>&&);
>     void deleteAllFonts();
>-    void releaseRemoteResource(WebCore::RenderingResourceIdentifier);
>+    void releaseRemoteResource(WebCore::RenderingResourceIdentifier, uint64_t useCount);
> 
>     enum class DidReceiveBackendCreationResult : bool {
>         ReceivedAnyResponse,
>diff --git a/Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp b/Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp
>index 03f34b33a22d..414e1996e8bc 100644
>--- a/Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp
>+++ b/Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp
>@@ -121,8 +121,7 @@ void RemoteResourceCacheProxy::releaseNativeImage(RenderingResourceIdentifier re
>     if (!m_nativeImages.remove(renderingResourceIdentifier))
>         return;
> 
>-    // Tell the GPU process to remove this resource.
>-    m_remoteRenderingBackendProxy.releaseRemoteResource(renderingResourceIdentifier);
>+    m_remoteRenderingBackendProxy.releaseRemoteResource(renderingResourceIdentifier, 0); // FIXME: Pass the real use count here.
> }
> 
> void RemoteResourceCacheProxy::prepareForNextRenderingUpdate()
>@@ -134,7 +133,7 @@ void RemoteResourceCacheProxy::prepareForNextRenderingUpdate()
> void RemoteResourceCacheProxy::clearFontMap()
> {
>     for (auto& item : m_fontIdentifierToLastRenderingUpdateVersionMap)
>-        m_remoteRenderingBackendProxy.releaseRemoteResource(item.key);
>+        m_remoteRenderingBackendProxy.releaseRemoteResource(item.key, 0); // FIXME: Pass the real use count here.
>     m_fontIdentifierToLastRenderingUpdateVersionMap.clear();
>     m_numberOfFontsUsedInCurrentRenderingUpdate = 0;
> }
>@@ -152,7 +151,7 @@ void RemoteResourceCacheProxy::finalizeRenderingUpdateForFonts()
>     for (auto& item : m_fontIdentifierToLastRenderingUpdateVersionMap) {
>         if (m_currentRenderingUpdateVersion - item.value >= minimumRenderingUpdateCountToKeepFontAlive) {
>             toRemove.add(item.key);
>-            m_remoteRenderingBackendProxy.releaseRemoteResource(item.key);
>+            m_remoteRenderingBackendProxy.releaseRemoteResource(item.key, 0); // FIXME: Pass the real use count here.
>         }
>     }
>

Actions: View | Formatted Diff | Diff

Attachments on bug 228222: 434067 | 434072 | 434094 | 434137 | 434138 | 434139 | 434145 | 434159 | 434264