Caching of generated images in CSS should be smarter.
authorakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 May 2013 17:34:36 +0000 (17:34 +0000)
committerakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 May 2013 17:34:36 +0000 (17:34 +0000)
<http://webkit.org/b/115902>
<rdar://problem/13542727>

Reviewed by Antti Koivisto.

Add an IntSize => GeneratorGeneratedImage cache at the CSSImageGeneratorValue level.

CSSGradientValue is currently the only CSSImageGeneratorValue subclass that makes use of the cache.
Generated images are kept for 3 seconds after last use.

The main problem with the previous approach was that background renderers (e.g <body>, <tr>, etc)
would be passed to multiple CSSImageGeneratorValue::getImage() calls with different sizes requested
for each of the descendent renderers that inherit their background from the same parent.
The cache wasn't smart enough for this, it just thought the background renderer was changing size
a lot, and would regenerate the image over and over.

We already had caching of intermediate image buffers for GeneratorGeneratedImage::drawPattern().
This removes the eviction timer from that cache so that the intermediate images can live a bit longer.

(WebCore::CSSImageGeneratorValue::cachedImageForSize):
(WebCore::CSSImageGeneratorValue::saveCachedImageForSize):

    Renamed from getImage() and putImage().

(WebCore::CSSImageGeneratorValue::evictCachedGeneratedImage):
(WebCore::CSSImageGeneratorValue::CachedGeneratedImage::CachedGeneratedImage):
(WebCore::CSSImageGeneratorValue::CachedGeneratedImage::evictionTimerFired):

    Let the CachedGeneratedImage throw itself out from cache when the timer fires.

* css/CSSImageGeneratorValue.h:
(CachedGeneratedImage):

    Exactly what it sounds like. These go into CSSImageGeneratorValue::m_images with the size
    as the hash key.

* platform/graphics/GeneratorGeneratedImage.cpp:
(WebCore::GeneratorGeneratedImage::drawPattern):
* platform/graphics/GeneratorGeneratedImage.h:
(WebCore::GeneratorGeneratedImage::~GeneratorGeneratedImage):
(WebCore::GeneratorGeneratedImage::GeneratorGeneratedImage):

    Keep the intermediate image for drawPattern() until destruction instead of dropping it on
    a timer. These objects are now evicted by the CSSImageGeneratorValue's image cache
    after 3 seconds of disuse rather than kept for the lifetime of the renderer.

* css/CSSCanvasValue.cpp:
(WebCore::CSSCanvasValue::canvasChanged):
(WebCore::CSSCanvasValue::canvasResized):
* css/CSSCrossfadeValue.cpp:
(WebCore::CSSCrossfadeValue::crossfadeChanged):
* rendering/style/StyleGeneratedImage.cpp:
(WebCore::StyleGeneratedImage::addClient):
* css/CSSImageGeneratorValue.cpp:
(WebCore::CSSImageGeneratorValue::addClient):
(WebCore::CSSImageGeneratorValue::removeClient):

    CSSImageGeneratorValue::m_clients is now a HashCountedSet<RenderObject*>, tweak accordingly.

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

Source/WebCore/ChangeLog
Source/WebCore/css/CSSCanvasValue.cpp
Source/WebCore/css/CSSCrossfadeValue.cpp
Source/WebCore/css/CSSGradientValue.cpp
Source/WebCore/css/CSSImageGeneratorValue.cpp
Source/WebCore/css/CSSImageGeneratorValue.h
Source/WebCore/platform/graphics/GeneratorGeneratedImage.cpp
Source/WebCore/platform/graphics/GeneratorGeneratedImage.h
Source/WebCore/rendering/style/StyleGeneratedImage.cpp
Tools/Scripts/webkitpy/port/mac.py

index 0202cd9..8688d70 100644 (file)
@@ -1,3 +1,65 @@
+2013-05-10  Andreas Kling  <akling@apple.com>
+
+        Caching of generated images in CSS should be smarter.
+        <http://webkit.org/b/115902>
+        <rdar://problem/13542727>
+
+        Reviewed by Antti Koivisto.
+
+        Add an IntSize => GeneratorGeneratedImage cache at the CSSImageGeneratorValue level.
+
+        CSSGradientValue is currently the only CSSImageGeneratorValue subclass that makes use of the cache.
+        Generated images are kept for 3 seconds after last use.
+
+        The main problem with the previous approach was that background renderers (e.g <body>, <tr>, etc)
+        would be passed to multiple CSSImageGeneratorValue::getImage() calls with different sizes requested
+        for each of the descendent renderers that inherit their background from the same parent.
+        The cache wasn't smart enough for this, it just thought the background renderer was changing size
+        a lot, and would regenerate the image over and over.
+
+        We already had caching of intermediate image buffers for GeneratorGeneratedImage::drawPattern().
+        This removes the eviction timer from that cache so that the intermediate images can live a bit longer.
+
+        (WebCore::CSSImageGeneratorValue::cachedImageForSize):
+        (WebCore::CSSImageGeneratorValue::saveCachedImageForSize):
+
+            Renamed from getImage() and putImage().
+
+        (WebCore::CSSImageGeneratorValue::evictCachedGeneratedImage):
+        (WebCore::CSSImageGeneratorValue::CachedGeneratedImage::CachedGeneratedImage):
+        (WebCore::CSSImageGeneratorValue::CachedGeneratedImage::evictionTimerFired):
+
+            Let the CachedGeneratedImage throw itself out from cache when the timer fires.
+
+        * css/CSSImageGeneratorValue.h:
+        (CachedGeneratedImage):
+
+            Exactly what it sounds like. These go into CSSImageGeneratorValue::m_images with the size
+            as the hash key.
+
+        * platform/graphics/GeneratorGeneratedImage.cpp:
+        (WebCore::GeneratorGeneratedImage::drawPattern):
+        * platform/graphics/GeneratorGeneratedImage.h:
+        (WebCore::GeneratorGeneratedImage::~GeneratorGeneratedImage):
+        (WebCore::GeneratorGeneratedImage::GeneratorGeneratedImage):
+
+            Keep the intermediate image for drawPattern() until destruction instead of dropping it on
+            a timer. These objects are now evicted by the CSSImageGeneratorValue's image cache
+            after 3 seconds of disuse rather than kept for the lifetime of the renderer.
+
+        * css/CSSCanvasValue.cpp:
+        (WebCore::CSSCanvasValue::canvasChanged):
+        (WebCore::CSSCanvasValue::canvasResized):
+        * css/CSSCrossfadeValue.cpp:
+        (WebCore::CSSCrossfadeValue::crossfadeChanged):
+        * rendering/style/StyleGeneratedImage.cpp:
+        (WebCore::StyleGeneratedImage::addClient):
+        * css/CSSImageGeneratorValue.cpp:
+        (WebCore::CSSImageGeneratorValue::addClient):
+        (WebCore::CSSImageGeneratorValue::removeClient):
+
+            CSSImageGeneratorValue::m_clients is now a HashCountedSet<RenderObject*>, tweak accordingly.
+
 2013-05-10  Anders Carlsson  <andersca@apple.com>
 
         Remove MemoryUsageSupport class
index 48c2626..e284100 100644 (file)
@@ -50,15 +50,15 @@ String CSSCanvasValue::customCssText() const
 void CSSCanvasValue::canvasChanged(HTMLCanvasElement*, const FloatRect& changedRect)
 {
     IntRect imageChangeRect = enclosingIntRect(changedRect);
-    RenderObjectSizeCountMap::const_iterator end = clients().end();
-    for (RenderObjectSizeCountMap::const_iterator curr = clients().begin(); curr != end; ++curr)
+    HashCountedSet<RenderObject*>::const_iterator end = clients().end();
+    for (HashCountedSet<RenderObject*>::const_iterator curr = clients().begin(); curr != end; ++curr)
         const_cast<RenderObject*>(curr->key)->imageChanged(static_cast<WrappedImagePtr>(this), &imageChangeRect);
 }
 
 void CSSCanvasValue::canvasResized(HTMLCanvasElement*)
 {
-    RenderObjectSizeCountMap::const_iterator end = clients().end();
-    for (RenderObjectSizeCountMap::const_iterator curr = clients().begin(); curr != end; ++curr)
+    HashCountedSet<RenderObject*>::const_iterator end = clients().end();
+    for (HashCountedSet<RenderObject*>::const_iterator curr = clients().begin(); curr != end; ++curr)
         const_cast<RenderObject*>(curr->key)->imageChanged(static_cast<WrappedImagePtr>(this));
 }
 
index 0cb07ce..736c0c4 100644 (file)
@@ -193,8 +193,8 @@ PassRefPtr<Image> CSSCrossfadeValue::image(RenderObject* renderer, const IntSize
 
 void CSSCrossfadeValue::crossfadeChanged(const IntRect&)
 {
-    RenderObjectSizeCountMap::const_iterator end = clients().end();
-    for (RenderObjectSizeCountMap::const_iterator curr = clients().begin(); curr != end; ++curr) {
+    HashCountedSet<RenderObject*>::const_iterator end = clients().end();
+    for (HashCountedSet<RenderObject*>::const_iterator curr = clients().begin(); curr != end; ++curr) {
         RenderObject* client = const_cast<RenderObject*>(curr->key);
         client->imageChanged(static_cast<WrappedImagePtr>(this));
     }
index 87411e2..660837f 100644 (file)
@@ -53,13 +53,11 @@ PassRefPtr<Image> CSSGradientValue::image(RenderObject* renderer, const IntSize&
         if (!clients().contains(renderer))
             return 0;
 
-        // Need to look up our size.  Create a string of width*height to use as a hash key.
-        Image* result = getImage(renderer, size);
+        Image* result = cachedImageForSize(size);
         if (result)
             return result;
     }
 
-    // We need to create an image.
     RefPtr<Gradient> gradient;
 
     if (isLinearGradient())
@@ -69,9 +67,9 @@ PassRefPtr<Image> CSSGradientValue::image(RenderObject* renderer, const IntSize&
         gradient = static_cast<CSSRadialGradientValue*>(this)->createGradient(renderer, size);
     }
 
-    RefPtr<Image> newImage = GeneratorGeneratedImage::create(gradient, size);
+    RefPtr<GeneratorGeneratedImage> newImage = GeneratorGeneratedImage::create(gradient, size);
     if (cacheable)
-        putImage(size, newImage);
+        saveCachedImageForSize(size, newImage);
 
     return newImage.release();
 }
index 1674c7e..b6e80b9 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008 Apple Inc.  All rights reserved.
+ * Copyright (C) 2008, 2011, 2012, 2013 Apple Inc.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -35,6 +35,8 @@
 
 namespace WebCore {
 
+static const double timeToKeepCachedGeneratedImagesInSeconds = 3;
+
 CSSImageGeneratorValue::CSSImageGeneratorValue(ClassType classType)
     : CSSValue(classType)
 {
@@ -44,68 +46,58 @@ CSSImageGeneratorValue::~CSSImageGeneratorValue()
 {
 }
 
-void CSSImageGeneratorValue::addClient(RenderObject* renderer, const IntSize& size)
+void CSSImageGeneratorValue::addClient(RenderObject* renderer)
 {
-    ref();
-
     ASSERT(renderer);
-    if (!size.isEmpty())
-        m_sizes.add(size);
-
-    RenderObjectSizeCountMap::iterator it = m_clients.find(renderer);
-    if (it == m_clients.end())
-        m_clients.add(renderer, SizeAndCount(size, 1));
-    else {
-        SizeAndCount& sizeCount = it->value;
-        ++sizeCount.count;
-    }
+    ref();
+    m_clients.add(renderer);
 }
 
 void CSSImageGeneratorValue::removeClient(RenderObject* renderer)
 {
     ASSERT(renderer);
-    RenderObjectSizeCountMap::iterator it = m_clients.find(renderer);
-    ASSERT(it != m_clients.end());
-
-    IntSize removedImageSize;
-    SizeAndCount& sizeCount = it->value;
-    IntSize size = sizeCount.size;
-    if (!size.isEmpty()) {
-        m_sizes.remove(size);
-        if (!m_sizes.contains(size))
-            m_images.remove(size);
-    }
-
-    if (!--sizeCount.count)
-        m_clients.remove(renderer);
-
+    m_clients.remove(renderer);
     deref();
 }
 
-Image* CSSImageGeneratorValue::getImage(RenderObject* renderer, const IntSize& size)
+GeneratorGeneratedImage* CSSImageGeneratorValue::cachedImageForSize(IntSize size)
 {
-    RenderObjectSizeCountMap::iterator it = m_clients.find(renderer);
-    if (it != m_clients.end()) {
-        SizeAndCount& sizeCount = it->value;
-        IntSize oldSize = sizeCount.size;
-        if (oldSize != size) {
-            RefPtr<CSSImageGeneratorValue> protect(this);
-            removeClient(renderer);
-            addClient(renderer, size);
-        }
-    }
-
-    // Don't generate an image for empty sizes.
     if (size.isEmpty())
         return 0;
 
-    // Look up the image in our cache.
-    return m_images.get(size);
+    CachedGeneratedImage* cachedGeneratedImage = m_images.get(size);
+    if (!cachedGeneratedImage)
+        return 0;
+
+    cachedGeneratedImage->puntEvictionTimer();
+    return cachedGeneratedImage->image();
+}
+
+void CSSImageGeneratorValue::saveCachedImageForSize(IntSize size, PassRefPtr<GeneratorGeneratedImage> image)
+{
+    ASSERT(!m_images.contains(size));
+    m_images.add(size, adoptPtr(new CachedGeneratedImage(*this, size, image)));
+}
+
+void CSSImageGeneratorValue::evictCachedGeneratedImage(IntSize size)
+{
+    ASSERT(m_images.contains(size));
+    m_images.remove(size);
+}
+
+CSSImageGeneratorValue::CachedGeneratedImage::CachedGeneratedImage(CSSImageGeneratorValue& owner, IntSize size, PassRefPtr<GeneratorGeneratedImage> image)
+    : m_owner(owner)
+    , m_size(size)
+    , m_image(image)
+    , m_evictionTimer(this, &CSSImageGeneratorValue::CachedGeneratedImage::evictionTimerFired, timeToKeepCachedGeneratedImagesInSeconds)
+{
+    m_evictionTimer.restart();
 }
 
-void CSSImageGeneratorValue::putImage(const IntSize& size, PassRefPtr<Image> image)
+void CSSImageGeneratorValue::CachedGeneratedImage::evictionTimerFired(DeferrableOneShotTimer<CachedGeneratedImage>*)
 {
-    m_images.add(size, image);
+    // NOTE: This is essentially a "delete this", the object is no longer valid after this line.
+    m_owner.evictCachedGeneratedImage(m_size);
 }
 
 PassRefPtr<Image> CSSImageGeneratorValue::image(RenderObject* renderer, const IntSize& size)
index 8c5ec77..4f54bbc 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008 Apple Inc. All rights reserved.
+ * Copyright (C) 2008, 2011, 2012, 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
 #define CSSImageGeneratorValue_h
 
 #include "CSSValue.h"
+#include "GeneratorGeneratedImage.h"
 #include "IntSizeHash.h"
+#include "Timer.h"
 #include <wtf/HashCountedSet.h>
 #include <wtf/RefPtr.h>
 
 namespace WebCore {
 
 class CachedResourceLoader;
-class Image;
+class GeneratorGeneratedImage;
 class RenderObject;
 class StyleResolver;
 
-struct SizeAndCount {
-    SizeAndCount(IntSize newSize = IntSize(), int newCount = 0)
-        : size(newSize)
-        , count(newCount)
-    {
-    }
-
-    IntSize size;
-    int count;
-};
-
-typedef HashMap<const RenderObject*, SizeAndCount> RenderObjectSizeCountMap;
-
 class CSSImageGeneratorValue : public CSSValue {
 public:
     ~CSSImageGeneratorValue();
 
-    void addClient(RenderObject*, const IntSize&);
+    void addClient(RenderObject*);
     void removeClient(RenderObject*);
+
     PassRefPtr<Image> image(RenderObject*, const IntSize&);
 
     bool isFixedSize() const;
@@ -70,13 +60,31 @@ public:
 protected:
     CSSImageGeneratorValue(ClassType);
 
-    Image* getImage(RenderObject*, const IntSize&);
-    void putImage(const IntSize&, PassRefPtr<Image>);
-    const RenderObjectSizeCountMap& clients() const { return m_clients; }
+    GeneratorGeneratedImage* cachedImageForSize(IntSize);
+    void saveCachedImageForSize(IntSize, PassRefPtr<GeneratorGeneratedImage>);
+    const HashCountedSet<RenderObject*>& clients() const { return m_clients; }
+
+private:
+    class CachedGeneratedImage {
+    public:
+        CachedGeneratedImage(CSSImageGeneratorValue&, IntSize, PassRefPtr<GeneratorGeneratedImage>);
+        GeneratorGeneratedImage* image() { return m_image.get(); }
+        void puntEvictionTimer() { m_evictionTimer.restart(); }
+
+    private:
+        void evictionTimerFired(DeferrableOneShotTimer<CachedGeneratedImage>*);
+
+        CSSImageGeneratorValue& m_owner;
+        IntSize m_size;
+        RefPtr<GeneratorGeneratedImage> m_image;
+        DeferrableOneShotTimer<CachedGeneratedImage> m_evictionTimer;
+    };
+
+    friend class CachedGeneratedImage;
+    void evictCachedGeneratedImage(IntSize);
 
-    HashCountedSet<IntSize> m_sizes; // A count of how many times a given image size is in use.
-    RenderObjectSizeCountMap m_clients; // A map from RenderObjects (with entry count) to image sizes.
-    HashMap<IntSize, RefPtr<Image> > m_images; // A cache of Image objects by image size.
+    HashCountedSet<RenderObject*> m_clients;
+    HashMap<IntSize, OwnPtr<CachedGeneratedImage> > m_images;
 };
 
 } // namespace WebCore
index 0647013..55b4735 100644 (file)
@@ -76,13 +76,6 @@ void GeneratorGeneratedImage::drawPattern(GraphicsContext* destContext, const Fl
 
     // Tile the image buffer into the context.
     m_cachedImageBuffer->drawPattern(destContext, adjustedSrcRect, adjustedPatternCTM, phase, styleColorSpace, compositeOp, destRect);
-    m_cacheTimer.restart();
-}
-
-void GeneratorGeneratedImage::invalidateCacheTimerFired(DeferrableOneShotTimer<GeneratorGeneratedImage>*)
-{
-    m_cachedImageBuffer.clear();
-    m_cachedAdjustedSize = IntSize();
 }
 
 }
index 47f51de..45e720f 100644 (file)
 #include "Image.h"
 #include "ImageBuffer.h"
 #include "IntSize.h"
-#include "Timer.h"
 #include <wtf/RefPtr.h>
 
 namespace WebCore {
 
-static const int generatedImageCacheClearDelay = 1;
-
 class GeneratorGeneratedImage : public GeneratedImage {
 public:
     static PassRefPtr<GeneratorGeneratedImage> create(PassRefPtr<Generator> generator, const IntSize& size)
@@ -45,29 +42,21 @@ public:
         return adoptRef(new GeneratorGeneratedImage(generator, size));
     }
 
-    virtual ~GeneratorGeneratedImage()
-    {
-        m_cacheTimer.stop();
-    }
+    virtual ~GeneratorGeneratedImage() { }
 
 protected:
     virtual void draw(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, ColorSpace styleColorSpace, CompositeOperator, BlendMode);
     virtual void drawPattern(GraphicsContext*, const FloatRect& srcRect, const AffineTransform& patternTransform,
         const FloatPoint& phase, ColorSpace styleColorSpace, CompositeOperator, const FloatRect& destRect, BlendMode);
 
-    void invalidateCacheTimerFired(DeferrableOneShotTimer<GeneratorGeneratedImage>*);
-
     GeneratorGeneratedImage(PassRefPtr<Generator> generator, const IntSize& size)
         : m_generator(generator)
-        , m_cacheTimer(this, &GeneratorGeneratedImage::invalidateCacheTimerFired, generatedImageCacheClearDelay)
     {
         m_size = size;
     }
 
     RefPtr<Generator> m_generator;
-
     OwnPtr<ImageBuffer> m_cachedImageBuffer;
-    DeferrableOneShotTimer<GeneratorGeneratedImage> m_cacheTimer;
     IntSize m_cachedAdjustedSize;
     unsigned m_cachedGeneratorHash;
 };
index c4c2f3b..b8161fe 100644 (file)
@@ -76,7 +76,7 @@ void StyleGeneratedImage::computeIntrinsicDimensions(const RenderObject* rendere
 
 void StyleGeneratedImage::addClient(RenderObject* renderer)
 {
-    m_imageGeneratorValue->addClient(renderer, IntSize());
+    m_imageGeneratorValue->addClient(renderer);
 }
 
 void StyleGeneratedImage::removeClient(RenderObject* renderer)
index d66e6d6..f2d1863 100644 (file)
@@ -136,6 +136,7 @@ class MacPort(ApplePort):
         return min(supportable_instances, default_count)
 
     def _build_java_test_support(self):
+        return True
         java_tests_path = self._filesystem.join(self.layout_tests_dir(), "java")
         build_java = [self.make_command(), "-C", java_tests_path]
         if self._executive.run_command(build_java, return_exit_code=True):  # Paths are absolute, so we don't need to set a cwd.