Purge cached SkPictureShader entries on shader deletion

We're currently adding picture shader cache entries to the resource
cache, but we don't ever purge.  To avoid exhausting the budget, add
logic to associate cached entries with their owning picture shader, and
purge when the shader is deleted.

-- Side note --

The current cache key is

 K(pictureID, ...)

so technically the cache entries are associated with the picture, not the
shader.  One could resonably argue we should only purge when the
*picture* is deleted.  Unfortunately, this doesn't work: the cache entries
contain indirect refs to the picture (SkImageShader -> SkImage_Generated
-> SkPictureImageGenerator -> SkPicture), so the picture is always kept
alive.

Associating the cache entries with the shader itself seems like a
reasonable alternative, even if we give up some cache persistence in the
process.

Change-Id: Ia115dbb5ae627e5ee171da7c4430fecfd42f4292
Reviewed-on: https://skia-review.googlesource.com/23380
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Florin Malita <fmalita@chromium.org>

2 files changed, 44 insertions, 12 deletions

diff --git a/src/shaders/SkPictureShader.cpp b/src/shaders/SkPictureShader.cpp
index 415d3e5713..5a0d70e5af 100644
--- a/src/shaders/SkPictureShader.cpp
+++ b/src/shaders/SkPictureShader.cpp
@@ -32,7 +32,7 @@ static unsigned gBitmapSkaderKeyNamespaceLabel;
 struct BitmapShaderKey : public SkResourceCache::Key {
 public:
     BitmapShaderKey(sk_sp<SkColorSpace> colorSpace,
-                    uint32_t pictureID,
+                    uint32_t shaderID,
                     const SkRect& tile,
                     SkShader::TileMode tmx,
                     SkShader::TileMode tmy,
@@ -40,7 +40,6 @@ public:
                     const SkMatrix& localMatrix,
                     SkTransferFunctionBehavior blendBehavior)
         : fColorSpace(std::move(colorSpace))
-        , fPictureID(pictureID)
         , fTile(tile)
         , fTmx(tmx)
         , fTmy(tmy)
@@ -52,7 +51,6 @@ public:
         }
 
         static const size_t keySize = sizeof(fColorSpace) +
-                                      sizeof(fPictureID) +
                                       sizeof(fTile) +
                                       sizeof(fTmx) + sizeof(fTmy) +
                                       sizeof(fScale) +
@@ -60,12 +58,23 @@ public:
                                       sizeof(fBlendBehavior);
         // This better be packed.
         SkASSERT(sizeof(uint32_t) * (&fEndOfStruct - (uint32_t*)&fColorSpace) == keySize);
-        this->init(&gBitmapSkaderKeyNamespaceLabel, 0, keySize);
+        this->init(&gBitmapSkaderKeyNamespaceLabel, MakeSharedID(shaderID), keySize);
+    }
+
+    static uint64_t MakeSharedID(uint32_t shaderID) {
+        uint64_t sharedID = SkSetFourByteTag('p', 's', 'd', 'r');
+        return (sharedID << 32) | shaderID;
     }
 
 private:
+    // TODO: there are some fishy things about using CS sk_sps in the key:
+    //   - false negatives: keys are memcmp'ed, so we don't detect equivalent CSs
+    //     (SkColorspace::Equals)
+    //   - we're keeping the CS alive, even when the client releases it
+    //
+    // Ideally we'd be using unique IDs or some other weak ref + purge mechanism
+    // when the CS is deleted.
     sk_sp<SkColorSpace>        fColorSpace;
-    uint32_t                   fPictureID;
     SkRect                     fTile;
     SkShader::TileMode         fTmx, fTmy;
     SkSize                     fScale;
@@ -104,6 +113,15 @@ struct BitmapShaderRec : public SkResourceCache::Rec {
     }
 };
 
+static int32_t gNextID = 1;
+uint32_t next_id() {
+    int32_t id;
+    do {
+        id = sk_atomic_inc(&gNextID);
+    } while (id == SK_InvalidGenID);
+    return static_cast<uint32_t>(id);
+}
+
 } // namespace
 
 SkPictureShader::SkPictureShader(sk_sp<SkPicture> picture, TileMode tmx, TileMode tmy,
@@ -115,7 +133,14 @@ SkPictureShader::SkPictureShader(sk_sp<SkPicture> picture, TileMode tmx, TileMod
     , fTmx(tmx)
     , fTmy(tmy)
     , fColorSpace(std::move(colorSpace))
-{}
+    , fUniqueID(next_id())
+    , fAddedToCache(false) {}
+
+SkPictureShader::~SkPictureShader() {
+    if (fAddedToCache.load()) {
+        SkResourceCache::PostPurgeSharedID(BitmapShaderKey::MakeSharedID(fUniqueID));
+    }
+}
 
 sk_sp<SkShader> SkPictureShader::Make(sk_sp<SkPicture> picture, TileMode tmx, TileMode tmy,
                                       const SkMatrix* localMatrix, const SkRect* tile) {
@@ -231,7 +256,7 @@ sk_sp<SkShader> SkPictureShader::refBitmapShader(const SkMatrix& viewMatrix, con
 
     sk_sp<SkShader> tileShader;
     BitmapShaderKey key(std::move(keyCS),
-                        fPicture->uniqueID(),
+                        fUniqueID,
                         fTile,
                         fTmx,
                         fTmy,
@@ -260,6 +285,7 @@ sk_sp<SkShader> SkPictureShader::refBitmapShader(const SkMatrix& viewMatrix, con
         tileShader = tileImage->makeShader(fTmx, fTmy, &shaderMatrix);
 
         SkResourceCache::Add(new BitmapShaderRec(key, tileShader.get()));
+        fAddedToCache.store(true);
     }
 
     return tileShader;
diff --git a/src/shaders/SkPictureShader.h b/src/shaders/SkPictureShader.h
index 55bbebbf81..87a5e40845 100644
--- a/src/shaders/SkPictureShader.h
+++ b/src/shaders/SkPictureShader.h
@@ -8,6 +8,7 @@
 #ifndef SkPictureShader_DEFINED
 #define SkPictureShader_DEFINED
 
+#include "SkAtomics.h"
 #include "SkShaderBase.h"
 
 class SkArenaAlloc;
@@ -22,6 +23,8 @@ class SkPicture;
  */
 class SkPictureShader : public SkShaderBase {
 public:
+    ~SkPictureShader() override;
+
     static sk_sp<SkShader> Make(sk_sp<SkPicture>, TileMode, TileMode, const SkMatrix*,
                                 const SkRect*);
 
@@ -49,10 +52,6 @@ private:
                                     SkColorSpace* dstColorSpace,
                                     const int maxTextureSize = 0) const;
 
-    sk_sp<SkPicture>    fPicture;
-    SkRect              fTile;
-    TileMode            fTmx, fTmy;
-
     class PictureShaderContext : public Context {
     public:
         PictureShaderContext(
@@ -70,9 +69,16 @@ private:
         typedef Context INHERITED;
     };
 
+    sk_sp<SkPicture>    fPicture;
+    SkRect              fTile;
+    TileMode            fTmx, fTmy;
+
     // Should never be set by a public constructor.  This is only used when onMakeColorSpace()
     // forces a deferred color space xform.
-    sk_sp<SkColorSpace>   fColorSpace;
+    sk_sp<SkColorSpace>    fColorSpace;
+
+    const uint32_t         fUniqueID;
+    mutable SkAtomic<bool> fAddedToCache;
 
     typedef SkShaderBase INHERITED;
 };