Don't create the SubimageCache just to clear an image from it
authorddkilzer@apple.com <ddkilzer@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 May 2018 00:50:16 +0000 (00:50 +0000)
committerddkilzer@apple.com <ddkilzer@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 May 2018 00:50:16 +0000 (00:50 +0000)
<https://webkit.org/b/185757>

Reviewed by Said Abou-Hallawa.

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.

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp
Source/WebCore/platform/graphics/cg/NativeImageCG.cpp
Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.cpp
Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h

index 2d702e7..c719b83 100644 (file)
@@ -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 Said Abou-Hallawa.
+
+        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-23  Eric Carlson  <eric.carlson@apple.com>
 
         Avoid loading AVFoundation to check supported MIME types if possible
index 895c90e..0b89d20 100644 (file)
@@ -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
index b08026b..987536b 100644 (file)
@@ -86,7 +86,7 @@ void clearNativeImageSubimages(const NativeImagePtr& image)
 {
 #if CACHE_SUBIMAGES
     if (image)
-        subimageCache().clearImage(image.get());
+        SubimageCacheWithTimer::clearImage(image.get());
 #endif
 }
 
index 33923ec..cfde29f 100644 (file)
 
 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;
 }
 
 }
index 9033e79..1a461ca 100644 (file)
@@ -80,22 +80,26 @@ public:
         static const bool safeToCompareToEmptyOrDeleted = true;
     };
 
-    typedef HashSet<SubimageCacheEntry, SubimageCacheHash, SubimageCacheEntryTraits> SubimageCache;
-
-public:
-    SubimageCacheWithTimer();
-    RetainPtr<CGImageRef> getSubimage(CGImageRef, const FloatRect&);
-    void clearImage(CGImageRef);
+    static RetainPtr<CGImageRef> getSubimage(CGImageRef, const FloatRect&);
+    static void clearImage(CGImageRef);
 
 private:
+    typedef HashSet<SubimageCacheEntry, SubimageCacheHash, SubimageCacheEntryTraits> SubimageCacheHashSet;
+
+    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