Attachment 341101 Details for Bug 185757 – Patch v4

[patch] Patch v4

bug-185757-20180523101820.patch (text/plain), 9.55 KB, created by David Kilzer (:ddkilzer) on 2018-05-23 10:18:21 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: David Kilzer (:ddkilzer)

Created: 2018-05-23 10:18:21 PDT

Size: 9.55 KB

patch

obsolete

>Subversion Revision: 231940
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 385dd61ab755a5d049b333bcb2ce9f0b359a9764..5c558f0f3f8854490782db966894708c6bb9f1cc 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,69 @@
>+2018-05-23  David Kilzer  <ddkilzer@apple.com>
>+
>+        Don't create the SubimageCache just to clear an image from it
>+        <https://webkit.org/b/185757>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        To fix this we make SubimageCacheWithTimer::clearImage() a
>+        static class method that checks whether the cache exists before
>+        removing it.  We also make SubimageCacheWithTimer::getImage() a
>+        static class method, and move more methods into the
>+        SubimageCacheWithTimer class and make them private to reduce API
>+        footprint.
>+
>+        * platform/graphics/cg/GraphicsContextCG.cpp:
>+        (WebCore::GraphicsContext::drawNativeImage): Switch to use new
>+        SubimageCacheWithTimer::getSubimage() static class method.
>+        * platform/graphics/cg/NativeImageCG.cpp:
>+        (WebCore::clearNativeImageSubimages): Switch to use new
>+        SubimageCacheWithTimer::clearImage() static class method which
>+        returns early if the subimage cache has not been created yet.
>+        This fixes the bug.
>+
>+        * platform/graphics/cg/SubimageCacheWithTimer.cpp:
>+        (WebCore::SubimageCacheWithTimer::s_cache): Allocate space for
>+        static class variable.
>+        (WebCore::SubimageCacheWithTimer::getSubimage): Replace instance
>+        method with new static class method that gets the subimage cache
>+        singleton and calls the subimage() instance method.
>+        (WebCore::SubimageCacheWithTimer::clearImage): Replace instance
>+        methdod with new static class method that returns early if the
>+        static cache singleton doesn't exist (fixes the bug), otherwise
>+        calls the clearImageAndSubimages() instance method.
>+        (WebCore::SubimageCacheWithTimer::subimage): Rename from
>+        getSubimage().  Use `auto` after renaming SubimageCache typedef
>+        to SubimageCacheHashSet.
>+        (WebCore::SubimageCacheWithTimer::clearImageAndSubimages):
>+        Rename from clearImage().  Modernize loops.
>+        (WebCore::SubimageCacheWithTimer::subimageCache): Change
>+        WebCore::subimageCache() to a static class method that creates
>+        the subimage cache singleton if it doesn't exist yet, and
>+        returns it.
>+        (WebCore::SubimageCacheWithTimer::subimageCacheExists): Add.
>+        Returns false if the subimage cache singleton has not been
>+        created yet.
>+
>+        * platform/graphics/cg/SubimageCacheWithTimer.h:
>+        - Rename typedef SubimageCache to SubimageCacheHashSet to avoid
>+          general confusion.
>+        (WebCore::SubimageCacheWithTimer::getSubimage):
>+        (WebCore::SubimageCacheWithTimer::clearImage):
>+        - Change to static class methods.
>+        (WebCore::SubimageCacheWithTimer::SubimageCacheWithTimer):
>+        - Make private.
>+        (WebCore::SubimageCacheWithTimer::subimage):
>+        - Rename from getSubimage() and make private.
>+        (WebCore::SubimageCacheWithTimer::clearImageAndSubimages):
>+        - Rename from clearImage() and make private.
>+        (WebCore::SubimageCacheWithTimer::subimageCache):
>+        - Rename from WebCore::subimageCache() and make a private static
>+          class method.
>+        (WebCore::SubimageCacheWithTimer::subimageCacheExists):
>+        - Add private static class method.
>+        (WebCore::SubimageCacheWithTimer::s_cache):
>+        - Declare private static variable to hold singleton.
>+
> 2018-05-17  David Kilzer  <ddkilzer@apple.com>
> 
>         Lazily create WebCore::Timer for WebCore::Image
>diff --git a/Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp b/Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp
>index 895c90ed10c13eaed66558f2ab78dcb9d6e77654..0b89d20c0fe4b06fa20426d503753955a8d919fa 100644
>--- a/Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp
>+++ b/Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp
>@@ -330,7 +330,7 @@ void GraphicsContext::drawNativeImage(const RetainPtr<CGImageRef>& image, const
>             adjustedDestRect.setHeight(subimageRect.height() / yScale);
> 
> #if CACHE_SUBIMAGES
>-            subImage = subimageCache().getSubimage(subImage.get(), subimageRect);
>+            subImage = SubimageCacheWithTimer::getSubimage(subImage.get(), subimageRect);
> #else
>             subImage = adoptCF(CGImageCreateWithImageInRect(subImage.get(), subimageRect));
> #endif
>diff --git a/Source/WebCore/platform/graphics/cg/NativeImageCG.cpp b/Source/WebCore/platform/graphics/cg/NativeImageCG.cpp
>index b08026bd1d77d2e2c5e4803832f65a1fd4fc68c9..987536bc88307506bee224292ef79ffad88b5938 100644
>--- a/Source/WebCore/platform/graphics/cg/NativeImageCG.cpp
>+++ b/Source/WebCore/platform/graphics/cg/NativeImageCG.cpp
>@@ -86,7 +86,7 @@ void clearNativeImageSubimages(const NativeImagePtr& image)
> {
> #if CACHE_SUBIMAGES
>     if (image)
>-        subimageCache().clearImage(image.get());
>+        SubimageCacheWithTimer::clearImage(image.get());
> #endif
> }
> 
>diff --git a/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.cpp b/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.cpp
>index 33923ec49011ac34ebf9d0751c9708617c138aaa..cfde29f1bf3930c6b4a54b92e9ba4fd33810df9d 100644
>--- a/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.cpp
>+++ b/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.cpp
>@@ -34,9 +34,22 @@
> 
> namespace WebCore {
> 
>+SubimageCacheWithTimer* SubimageCacheWithTimer::s_cache;
>+
> static const Seconds subimageCacheClearDelay { 1_s };
> static const int maxSubimageCacheSize = 300;
> 
>+RetainPtr<CGImageRef> SubimageCacheWithTimer::getSubimage(CGImageRef image, const FloatRect& rect)
>+{
>+    return subimageCache().subimage(image, rect);
>+}
>+
>+void SubimageCacheWithTimer::clearImage(CGImageRef image)
>+{
>+    if (subimageCacheExists())
>+        subimageCache().clearImageAndSubimages(image);
>+}
>+
> struct SubimageRequest {
>     CGImageRef image;
>     const FloatRect& rect;
>@@ -73,7 +86,7 @@ void SubimageCacheWithTimer::invalidateCacheTimerFired()
>     m_cache.clear();
> }
> 
>-RetainPtr<CGImageRef> SubimageCacheWithTimer::getSubimage(CGImageRef image, const FloatRect& rect)
>+RetainPtr<CGImageRef> SubimageCacheWithTimer::subimage(CGImageRef image, const FloatRect& rect)
> {
>     m_timer.restart();
>     if (m_cache.size() == maxSubimageCacheSize) {
>@@ -83,34 +96,39 @@ RetainPtr<CGImageRef> SubimageCacheWithTimer::getSubimage(CGImageRef image, cons
>     }
> 
>     ASSERT(m_cache.size() < maxSubimageCacheSize);
>-    SubimageCache::AddResult result = m_cache.add<SubimageCacheAdder>(SubimageRequest(image, rect));
>+    auto result = m_cache.add<SubimageCacheAdder>(SubimageRequest(image, rect));
>     if (result.isNewEntry)
>         m_images.add(image);
> 
>     return result.iterator->subimage;
> }
> 
>-void SubimageCacheWithTimer::clearImage(CGImageRef image)
>+void SubimageCacheWithTimer::clearImageAndSubimages(CGImageRef image)
> {
>     if (m_images.contains(image)) {
>         Vector<SubimageCacheEntry> toBeRemoved;
>-        SubimageCache::const_iterator end = m_cache.end();
>-        for (SubimageCache::const_iterator it = m_cache.begin(); it != end; ++it) {
>-            if (it->image.get() == image)
>-                toBeRemoved.append(*it);
>+        for (const auto& entry : m_cache) {
>+            if (entry.image.get() == image)
>+                toBeRemoved.append(entry);
>         }
> 
>-        for (Vector<SubimageCacheEntry>::iterator removeIt = toBeRemoved.begin(); removeIt != toBeRemoved.end(); ++removeIt)
>-            m_cache.remove(*removeIt);
>+        for (auto& entry : toBeRemoved)
>+            m_cache.remove(entry);
> 
>         m_images.removeAll(image);
>     }
> }
> 
>-SubimageCacheWithTimer& subimageCache()
>+SubimageCacheWithTimer& SubimageCacheWithTimer::subimageCache()
>+{
>+    if (!s_cache)
>+        s_cache = new SubimageCacheWithTimer;
>+    return *s_cache;
>+}
>+
>+bool SubimageCacheWithTimer::subimageCacheExists()
> {
>-    static SubimageCacheWithTimer& cache = *new SubimageCacheWithTimer;
>-    return cache;
>+    return !!s_cache;
> }
> 
> }
>diff --git a/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h b/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h
>index 9033e797372335afffc712dd36f71ce30593af54..fd6dfe7583f99a1557798f92bbf655ac8a8cc778 100644
>--- a/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h
>+++ b/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h
>@@ -80,22 +80,26 @@ public:
>         static const bool safeToCompareToEmptyOrDeleted = true;
>     };
> 
>-    typedef HashSet<SubimageCacheEntry, SubimageCacheHash, SubimageCacheEntryTraits> SubimageCache;
>+    typedef HashSet<SubimageCacheEntry, SubimageCacheHash, SubimageCacheEntryTraits> SubimageCacheHashSet;
> 
>-public:
>-    SubimageCacheWithTimer();
>-    RetainPtr<CGImageRef> getSubimage(CGImageRef, const FloatRect&);
>-    void clearImage(CGImageRef);
>+    static RetainPtr<CGImageRef> getSubimage(CGImageRef, const FloatRect&);
>+    static void clearImage(CGImageRef);
> 
> private:
>+    SubimageCacheWithTimer();
>     void invalidateCacheTimerFired();
> 
>+    RetainPtr<CGImageRef> subimage(CGImageRef, const FloatRect&);
>+    void clearImageAndSubimages(CGImageRef);
>+
>     HashCountedSet<CGImageRef> m_images;
>-    SubimageCache m_cache;
>+    SubimageCacheHashSet m_cache;
>     DeferrableOneShotTimer m_timer;
>-};
> 
>-SubimageCacheWithTimer& subimageCache();
>+    static SubimageCacheWithTimer& subimageCache();
>+    static bool subimageCacheExists();
>+    static SubimageCacheWithTimer* s_cache;
>+};
> 
> #endif // CACHE_SUBIMAGES
>

Actions: View | Formatted Diff | Diff

Attachments on bug 185757: 340676 | 340700 | 340863 | 340952 | 341101