Attachment 434070 Details for Bug 228224 – Patch for reviewing

[patch] Patch for reviewing

patch.diff (text/plain), 18.43 KB, created by Myles C. Maxfield on 2021-07-23 00:45:09 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Myles C. Maxfield

Created: 2021-07-23 00:45:09 PDT

Size: 18.43 KB

patch

obsolete

>commit a89b68d19a867569a3ce590e5cea162cd7b8300a
>Author: Myles C. Maxfield <mmaxfield@apple.com>
>Date:   Fri Jul 23 00:36:36 2021 -0700
>
>    Source/WebCore:
>    Unreviewed, reverting 280130.
>    https://bugs.webkit.org/show_bug.cgi?id=228212
>    
>    Introduced MotionMark regression
>    
>    Reverted changeset:
>    
>    Patch by Said Abou-Hallawa <said@apple.com> on 2021-07-22
>    
>    * rendering/RenderBlock.cpp:
>    (WebCore::RenderBlock::simplifiedLayout):
>    
>    Source/WebKit:
>    [GPU Process] Start tracking resource uses for NativeImages and Fonts
>    https://bugs.webkit.org/show_bug.cgi?id=228224
>    
>    Reviewed by NOBODY (OOPS!).
>    
>    This is part 3 of https://bugs.webkit.org/show_bug.cgi?id=228216.
>    
>    This patch triggers the infrastructure added in https://bugs.webkit.org/show_bug.cgi?id=228222 by tracking
>    resource use counts in the web process. RemoteResourceCacheProxy already is notified whenever a resource
>    is used, so it records how many of these notifications it receives, and sends that count to the GPU process
>    with the release message. This patch extends the existing resource tracking HashMaps used in
>    RemoteResourceCacheProxy to continue to record what they were recording before, but also include this new
>    resource use count information.
>    
>    Covered by existing tests.
>    
>    * GPUProcess/graphics/RemoteRenderingBackend.cpp:
>    (WebKit::RemoteRenderingBackend::ReplayerDelegate::apply):
>    * WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:
>    (WebKit::RemoteResourceCacheProxy::~RemoteResourceCacheProxy):
>    (WebKit::RemoteResourceCacheProxy::cacheNativeImage):
>    (WebKit::RemoteResourceCacheProxy::cacheFont):
>    (WebKit::RemoteResourceCacheProxy::releaseNativeImage):
>    (WebKit::RemoteResourceCacheProxy::clearFontMap):
>    (WebKit::RemoteResourceCacheProxy::finalizeRenderingUpdateForFonts):
>    * WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:
>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 49b73a738c50..8e5cba30ceef 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,29 +1,3 @@
>-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/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 0f37f4aa2ea4..f36c9d5b915a 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,55 +1,31 @@
>-2021-07-22  Myles C. Maxfield  <mmaxfield@apple.com>
>+2021-07-23  Myles C. Maxfield  <mmaxfield@apple.com>
> 
>-        [GPU Process] Add resource use counter infrastructure to RemoteResourceCache
>-        https://bugs.webkit.org/show_bug.cgi?id=228222
>+        [GPU Process] Start tracking resource uses for NativeImages and Fonts
>+        https://bugs.webkit.org/show_bug.cgi?id=228224
> 
>         Reviewed by NOBODY (OOPS!).
> 
>-        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.
>+        This is part 3 of https://bugs.webkit.org/show_bug.cgi?id=228216.
>+
>+        This patch triggers the infrastructure added in https://bugs.webkit.org/show_bug.cgi?id=228222 by tracking
>+        resource use counts in the web process. RemoteResourceCacheProxy already is notified whenever a resource
>+        is used, so it records how many of these notifications it receives, and sends that count to the GPU process
>+        with the release message. This patch extends the existing resource tracking HashMaps used in
>+        RemoteResourceCacheProxy to continue to record what they were recording before, but also include this new 
>+        resource use count information.
>+
>+        Covered by existing tests.
> 
>         * 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.
>+        (WebKit::RemoteRenderingBackend::ReplayerDelegate::apply):
>         * 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.
>+        (WebKit::RemoteResourceCacheProxy::~RemoteResourceCacheProxy):
>+        (WebKit::RemoteResourceCacheProxy::cacheNativeImage):
>+        (WebKit::RemoteResourceCacheProxy::cacheFont):
>+        (WebKit::RemoteResourceCacheProxy::releaseNativeImage):
>+        (WebKit::RemoteResourceCacheProxy::clearFontMap):
>+        (WebKit::RemoteResourceCacheProxy::finalizeRenderingUpdateForFonts):
>+        * WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:
> 
> 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 c106f42cc4f3..491ca10bdb0c 100644
>--- a/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp
>+++ b/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp
>@@ -193,7 +193,22 @@ RemoteRenderingBackend::ReplayerDelegate::ReplayerDelegate(WebCore::ImageBuffer&
> 
> bool RemoteRenderingBackend::ReplayerDelegate::apply(WebCore::DisplayList::ItemHandle item, WebCore::GraphicsContext& graphicsContext)
> {
>-    // FIXME: Inspect the item and record resource use here.
>+    auto& remoteResourceCache = m_remoteRenderingBackend.remoteResourceCache();
>+
>+    if (item.is<WebCore::DisplayList::DrawGlyphs>())
>+        remoteResourceCache.recordResourceUse(item.get<WebCore::DisplayList::DrawGlyphs>().fontIdentifier());
>+    else if (item.is<WebCore::DisplayList::DrawPattern>())
>+        remoteResourceCache.recordResourceUse(item.get<WebCore::DisplayList::DrawPattern>().imageIdentifier());
>+    else if (item.is<WebCore::DisplayList::DrawNativeImage>())
>+        remoteResourceCache.recordResourceUse(item.get<WebCore::DisplayList::DrawNativeImage>().imageIdentifier());
>+    else if (item.is<WebCore::DisplayList::SetState>()) {
>+        auto& setState = item.get<WebCore::DisplayList::SetState>();
>+        auto changeFlags = setState.stateChange().m_changeFlags;
>+        if (changeFlags.contains(GraphicsContextState::StrokePatternChange))
>+            remoteResourceCache.recordResourceUse(setState.strokePatternImageIdentifier());
>+        if (changeFlags.contains(GraphicsContextState::FillPatternChange))
>+            remoteResourceCache.recordResourceUse(setState.fillPatternImageIdentifier());
>+    }
> 
>     auto apply = [&](auto&& destination) {
>         return destination.apply(item, graphicsContext);
>diff --git a/Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp b/Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp
>index 414e1996e8bc..40ec392cae46 100644
>--- a/Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp
>+++ b/Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp
>@@ -41,8 +41,8 @@ RemoteResourceCacheProxy::RemoteResourceCacheProxy(RemoteRenderingBackendProxy&
> 
> RemoteResourceCacheProxy::~RemoteResourceCacheProxy()
> {
>-    for (auto& image : m_nativeImages.values())
>-        image->removeObserver(*this);
>+    for (auto& nativeImageState : m_nativeImages.values())
>+        nativeImageState.image->removeObserver(*this);
> }
> 
> void RemoteResourceCacheProxy::cacheImageBuffer(WebCore::ImageBuffer& imageBuffer)
>@@ -80,8 +80,11 @@ inline static RefPtr<ShareableBitmap> createShareableBitmapFromNativeImage(Nativ
> 
> void RemoteResourceCacheProxy::cacheNativeImage(NativeImage& image)
> {
>-    if (m_nativeImages.contains(image.renderingResourceIdentifier()))
>+    auto iterator = m_nativeImages.find(image.renderingResourceIdentifier());
>+    if (iterator != m_nativeImages.end()) {
>+        ++iterator->value.useCount;
>         return;
>+    }
> 
>     auto bitmap = createShareableBitmapFromNativeImage(image);
>     if (!bitmap)
>@@ -92,7 +95,7 @@ void RemoteResourceCacheProxy::cacheNativeImage(NativeImage& image)
>     if (handle.isNull())
>         return;
> 
>-    m_nativeImages.add(image.renderingResourceIdentifier(), makeWeakPtr(image));
>+    m_nativeImages.add(image.renderingResourceIdentifier(), NativeImageState { makeWeakPtr(image), 1 });
> 
>     // Set itself as an observer to NativeImage, so releaseNativeImage()
>     // gets called when NativeImage is being deleleted.
>@@ -104,24 +107,30 @@ void RemoteResourceCacheProxy::cacheNativeImage(NativeImage& image)
> 
> void RemoteResourceCacheProxy::cacheFont(Font& font)
> {
>-    auto result = m_fontIdentifierToLastRenderingUpdateVersionMap.ensure(font.renderingResourceIdentifier(), [&] {
>-        return 0;
>+    auto result = m_fonts.ensure(font.renderingResourceIdentifier(), [&] {
>+        return FontState { 0, 0 };
>     });
>-    auto& lastVersion = result.iterator->value;
>-    if (lastVersion != m_currentRenderingUpdateVersion) {
>-        lastVersion = m_currentRenderingUpdateVersion;
>+    auto& currentState = result.iterator->value;
>+    if (currentState.lastRenderingUpdateVersionUsedWithin != m_currentRenderingUpdateVersion) {
>+        currentState.lastRenderingUpdateVersionUsedWithin = m_currentRenderingUpdateVersion;
>         ++m_numberOfFontsUsedInCurrentRenderingUpdate;
>     }
>+    ++currentState.useCount;
>     if (result.isNewEntry)
>         m_remoteRenderingBackendProxy.cacheFont(makeRef(font));
> }
> 
> void RemoteResourceCacheProxy::releaseNativeImage(RenderingResourceIdentifier renderingResourceIdentifier)
> {
>-    if (!m_nativeImages.remove(renderingResourceIdentifier))
>-        return;
>+    auto iterator = m_nativeImages.find(renderingResourceIdentifier);
>+    RELEASE_ASSERT(iterator != m_nativeImages.end());
>+
>+    auto useCount = iterator->value.useCount;
>+
>+    auto success = m_nativeImages.remove(iterator);
>+    ASSERT_UNUSED(success, success);
> 
>-    m_remoteRenderingBackendProxy.releaseRemoteResource(renderingResourceIdentifier, 0); // FIXME: Pass the real use count here.
>+    m_remoteRenderingBackendProxy.releaseRemoteResource(renderingResourceIdentifier, useCount);
> }
> 
> void RemoteResourceCacheProxy::prepareForNextRenderingUpdate()
>@@ -132,9 +141,9 @@ void RemoteResourceCacheProxy::prepareForNextRenderingUpdate()
> 
> void RemoteResourceCacheProxy::clearFontMap()
> {
>-    for (auto& item : m_fontIdentifierToLastRenderingUpdateVersionMap)
>-        m_remoteRenderingBackendProxy.releaseRemoteResource(item.key, 0); // FIXME: Pass the real use count here.
>-    m_fontIdentifierToLastRenderingUpdateVersionMap.clear();
>+    for (auto& fontState : m_fonts)
>+        m_remoteRenderingBackendProxy.releaseRemoteResource(fontState.key, fontState.value.useCount);
>+    m_fonts.clear();
>     m_numberOfFontsUsedInCurrentRenderingUpdate = 0;
> }
> 
>@@ -142,20 +151,20 @@ void RemoteResourceCacheProxy::finalizeRenderingUpdateForFonts()
> {
>     static constexpr unsigned minimumRenderingUpdateCountToKeepFontAlive = 4;
> 
>-    unsigned totalFontCount = m_fontIdentifierToLastRenderingUpdateVersionMap.size();
>+    unsigned totalFontCount = m_fonts.size();
>     RELEASE_ASSERT(m_numberOfFontsUsedInCurrentRenderingUpdate <= totalFontCount);
>     if (totalFontCount == m_numberOfFontsUsedInCurrentRenderingUpdate)
>         return;
> 
>     HashSet<WebCore::RenderingResourceIdentifier> toRemove;
>-    for (auto& item : m_fontIdentifierToLastRenderingUpdateVersionMap) {
>-        if (m_currentRenderingUpdateVersion - item.value >= minimumRenderingUpdateCountToKeepFontAlive) {
>+    for (auto& item : m_fonts) {
>+        if (m_currentRenderingUpdateVersion - item.value.lastRenderingUpdateVersionUsedWithin >= minimumRenderingUpdateCountToKeepFontAlive) {
>             toRemove.add(item.key);
>-            m_remoteRenderingBackendProxy.releaseRemoteResource(item.key, 0); // FIXME: Pass the real use count here.
>+            m_remoteRenderingBackendProxy.releaseRemoteResource(item.key, item.value.useCount);
>         }
>     }
> 
>-    m_fontIdentifierToLastRenderingUpdateVersionMap.removeIf([&](const auto& bucket) {
>+    m_fonts.removeIf([&](const auto& bucket) {
>         return toRemove.contains(bucket.key);
>     });
> }
>diff --git a/Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h b/Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h
>index 6d731c4cc5ef..8d1119cab90e 100644
>--- a/Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h
>+++ b/Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h
>@@ -50,8 +50,8 @@ public:
>     void releaseImageBuffer(WebCore::RenderingResourceIdentifier);
> 
>     void cacheNativeImage(WebCore::NativeImage&);
>-
>     void cacheFont(WebCore::Font&);
>+
>     void didFinalizeRenderingUpdate();
> 
>     void remoteResourceCacheWasDestroyed();
>@@ -59,7 +59,18 @@ public:
> 
> private:
>     using ImageBufferHashMap = HashMap<WebCore::RenderingResourceIdentifier, WeakPtr<WebCore::ImageBuffer>>;
>-    using NativeImageHashMap = HashMap<WebCore::RenderingResourceIdentifier, WeakPtr<WebCore::NativeImage>>;
>+
>+    struct NativeImageState {
>+        WeakPtr<WebCore::NativeImage> image;
>+        uint64_t useCount;
>+    };
>+    using NativeImageHashMap = HashMap<WebCore::RenderingResourceIdentifier, NativeImageState>;
>+
>+    struct FontState {
>+        uint64_t lastRenderingUpdateVersionUsedWithin;
>+        uint64_t useCount;
>+    };
>+    using FontHashMap = HashMap<WebCore::RenderingResourceIdentifier, FontState>;
>     
>     void releaseNativeImage(WebCore::RenderingResourceIdentifier) override;
>     void finalizeRenderingUpdateForFonts();
>@@ -68,8 +79,7 @@ private:
> 
>     ImageBufferHashMap m_imageBuffers;
>     NativeImageHashMap m_nativeImages;
>-
>-    HashMap<WebCore::RenderingResourceIdentifier, uint64_t> m_fontIdentifierToLastRenderingUpdateVersionMap;
>+    FontHashMap m_fonts;
>     unsigned m_numberOfFontsUsedInCurrentRenderingUpdate { 0 };
>     uint64_t m_currentRenderingUpdateVersion { 1 };
>

Actions: View | Formatted Diff | Diff

Attachments on bug 228224: 434070 | 434071 | 434267 | 434309