Revert of Change SkResourceCache to take a Visitor inside its find(). (patchset ...
authorreed <reed@google.com>
Mon, 15 Sep 2014 17:15:18 +0000 (10:15 -0700)
committerCommit bot <commit-bot@chromium.org>
Mon, 15 Sep 2014 17:15:18 +0000 (10:15 -0700)
Reason for revert:
crashes on android bots, haven't diagnosed yet

Original issue's description:
> Change SkResourceCache to take a Visitor inside its find().
>
> This simplifies the API/contract, in that there are not any exposed
> lock/unlock scopes.
>
>
> patch from issue 572573002
>
> BUG=skia:
>
> Committed: https://skia.googlesource.com/skia/+/dee6a8e67db39fcbde2b3bb09be1d088ebb9db8a

R=mtklein@google.com, danakj@chromium.org
TBR=danakj@chromium.org, mtklein@google.com
NOTREECHECKS=true
NOTRY=true
BUG=skia:

Author: reed@google.com

Review URL: https://codereview.chromium.org/569303002

bench/ImageCacheBench.cpp
src/core/SkBitmapCache.cpp
src/core/SkResourceCache.cpp
src/core/SkResourceCache.h
tests/ImageCacheTest.cpp

index 0f8fdf27089916b0df25d0aa49a0f60078b3fd39..ca2b9342d515ba3186a0a8b4670a69d836561c1a 100644 (file)
@@ -27,10 +27,6 @@ struct TestRec : public SkResourceCache::Rec {
 
     virtual const Key& getKey() const SK_OVERRIDE { return fKey; }
     virtual size_t bytesUsed() const SK_OVERRIDE { return sizeof(fKey) + sizeof(fValue); }
-
-    static bool Visitor(const SkResourceCache::Rec&, void*) {
-        return true;
-    }
 };
 }
 
@@ -45,7 +41,7 @@ public:
 
     void populateCache() {
         for (int i = 0; i < CACHE_COUNT; ++i) {
-            fCache.add(SkNEW_ARGS(TestRec, (TestKey(i), i)));
+            fCache.unlock(fCache.addAndLock(SkNEW_ARGS(TestRec, (TestKey(i), i))));
         }
     }
 
@@ -62,8 +58,8 @@ protected:
         TestKey key(-1);
         // search for a miss (-1)
         for (int i = 0; i < loops; ++i) {
-            SkDEBUGCODE(bool found =) fCache.find(key, TestRec::Visitor, NULL);
-            SkASSERT(!found);
+            SkDEBUGCODE(SkResourceCache::ID id =) fCache.findAndLock(key);
+            SkASSERT(NULL == id);
         }
     }
 
index 84f10363f02aac1bd9b62e9f0c7db7f401e29af7..6d4f4b4cc7d7349173f34048e9ba36a16cd1c508 100644 (file)
@@ -59,16 +59,25 @@ struct BitmapRec : public SkResourceCache::Rec {
 
     virtual const Key& getKey() const SK_OVERRIDE { return fKey; }
     virtual size_t bytesUsed() const SK_OVERRIDE { return sizeof(fKey) + fBitmap.getSize(); }
+};
 
-    static bool Visitor(const SkResourceCache::Rec& baseRec, void* contextBitmap) {
-        const BitmapRec& rec = static_cast<const BitmapRec&>(baseRec);
-        SkBitmap* result = (SkBitmap*)contextBitmap;
+static bool find_and_return(const BitmapKey& key, SkBitmap* result) {
+    const BitmapRec* rec = (BitmapRec*)SkResourceCache::FindAndLock(key);
+    if (rec) {
+        *result = rec->fBitmap;
+        SkResourceCache::Unlock(rec);
 
-        *result = rec.fBitmap;
         result->lockPixels();
-        return SkToBool(result->getPixels());
+        if (result->getPixels()) {
+            return true;
+        }
+
+        SkResourceCache::Remove(rec);
+        result->reset();
+        // fall-through to false
     }
-};
+    return false;
+}
 
 bool SkBitmapCache::Find(const SkBitmap& src, SkScalar invScaleX, SkScalar invScaleY,
                          SkBitmap* result) {
@@ -77,7 +86,7 @@ bool SkBitmapCache::Find(const SkBitmap& src, SkScalar invScaleX, SkScalar invSc
         return false;
     }
     BitmapKey key(src.getGenerationID(), invScaleX, invScaleY, get_bounds_from_bitmap(src));
-    return SkResourceCache::Find(key, BitmapRec::Visitor, result);
+    return find_and_return(key, result);
 }
 
 void SkBitmapCache::Add(const SkBitmap& src, SkScalar invScaleX, SkScalar invScaleY,
@@ -93,7 +102,7 @@ void SkBitmapCache::Add(const SkBitmap& src, SkScalar invScaleX, SkScalar invSca
 
 bool SkBitmapCache::Find(uint32_t genID, const SkIRect& subset, SkBitmap* result) {
     BitmapKey key(genID, SK_Scalar1, SK_Scalar1, subset);
-    return SkResourceCache::Find(key, BitmapRec::Visitor, result);
+    return find_and_return(key, result);
 }
 
 bool SkBitmapCache::Add(uint32_t genID, const SkIRect& subset, const SkBitmap& result) {
@@ -129,22 +138,16 @@ struct MipMapRec : public SkResourceCache::Rec {
 
     virtual const Key& getKey() const SK_OVERRIDE { return fKey; }
     virtual size_t bytesUsed() const SK_OVERRIDE { return sizeof(fKey) + fMipMap->getSize(); }
-
-    static bool Visitor(const SkResourceCache::Rec& baseRec, void* contextMip) {
-        const MipMapRec& rec = static_cast<const MipMapRec&>(baseRec);
-        const SkMipMap** result = (const SkMipMap**)contextMip;
-        
-        *result = SkRef(rec.fMipMap);
-        // mipmaps don't use the custom allocator yet, so we don't need to check pixels
-        return true;
-    }
 };
 
+
 const SkMipMap* SkMipMapCache::FindAndRef(const SkBitmap& src) {
     BitmapKey key(src.getGenerationID(), 0, 0, get_bounds_from_bitmap(src));
-    const SkMipMap* result;
-    if (!SkResourceCache::Find(key, MipMapRec::Visitor, &result)) {
-        result = NULL;
+    const MipMapRec* rec = (MipMapRec*)SkResourceCache::FindAndLock(key);
+    const SkMipMap* result = NULL;
+    if (rec) {
+        result = SkRef(rec->fMipMap);
+        SkResourceCache::Unlock(rec);
     }
     return result;
 }
index 85b5b907b11f08e705af4af0b0cd82451922cfb4..68248f512bd2f08e4575e06d916e462750578d1f 100644 (file)
@@ -187,34 +187,80 @@ SkResourceCache::~SkResourceCache() {
 
 ////////////////////////////////////////////////////////////////////////////////
 
-bool SkResourceCache::find(const Key& key, VisitorProc visitor, void* context) {
+const SkResourceCache::Rec* SkResourceCache::findAndLock(const Key& key) {
     Rec* rec = fHash->find(key);
     if (rec) {
-        if (visitor(*rec, context)) {
-            this->moveToHead(rec);  // for our LRU
-            return true;
-        } else {
-            this->remove(rec);  // stale
-            return false;
-        }
+        this->moveToHead(rec);  // for our LRU
+        rec->fLockCount += 1;
     }
-    return false;
+    return rec;
+}
+
+const SkResourceCache::Rec* SkResourceCache::addAndLock(Rec* rec) {
+    SkASSERT(rec);
+    // See if we already have this key (racy inserts, etc.)
+    const Rec* existing = this->findAndLock(rec->getKey());
+    if (existing) {
+        SkDELETE(rec);
+        return existing;
+    }
+
+    this->addToHead(rec);
+    SkASSERT(1 == rec->fLockCount);
+    fHash->add(rec);
+    // We may (now) be overbudget, so see if we need to purge something.
+    this->purgeAsNeeded();
+    return rec;
 }
 
 void SkResourceCache::add(Rec* rec) {
     SkASSERT(rec);
     // See if we already have this key (racy inserts, etc.)
-    Rec* existing = fHash->find(rec->getKey());
+    const Rec* existing = this->findAndLock(rec->getKey());
     if (existing) {
         SkDELETE(rec);
+        this->unlock(existing);
         return;
     }
     
     this->addToHead(rec);
+    SkASSERT(1 == rec->fLockCount);
     fHash->add(rec);
+    this->unlock(rec);
+}
+
+void SkResourceCache::unlock(SkResourceCache::ID id) {
+    SkASSERT(id);
+
+#ifdef SK_DEBUG
+    {
+        bool found = false;
+        Rec* rec = fHead;
+        while (rec != NULL) {
+            if (rec == id) {
+                found = true;
+                break;
+            }
+            rec = rec->fNext;
+        }
+        SkASSERT(found);
+    }
+#endif
+    const Rec* rec = id;
+    SkASSERT(rec->fLockCount > 0);
+    // We're under our lock, and we're the only possible mutator, so unconsting is fine.
+    const_cast<Rec*>(rec)->fLockCount -= 1;
+
+    // we may have been over-budget, but now have released something, so check
+    // if we should purge.
+    if (0 == rec->fLockCount) {
+        this->purgeAsNeeded();
+    }
 }
 
 void SkResourceCache::remove(Rec* rec) {
+    SkASSERT(0 == rec->fLockCount);
+
     size_t used = rec->bytesUsed();
     SkASSERT(used <= fTotalBytesUsed);
 
@@ -246,7 +292,9 @@ void SkResourceCache::purgeAsNeeded(bool forcePurge) {
         }
 
         Rec* prev = rec->fPrev;
-        this->remove(rec);
+        if (0 == rec->fLockCount) {
+            this->remove(rec);
+        }
         rec = prev;
     }
 }
@@ -369,8 +417,16 @@ void SkResourceCache::validate() const {
 void SkResourceCache::dump() const {
     this->validate();
 
-    SkDebugf("SkResourceCache: count=%d bytes=%d %s\n",
-             fCount, fTotalBytesUsed, fDiscardableFactory ? "discardable" : "malloc");
+    const Rec* rec = fHead;
+    int locked = 0;
+    while (rec) {
+        locked += rec->fLockCount > 0;
+        rec = rec->fNext;
+    }
+
+    SkDebugf("SkResourceCache: count=%d bytes=%d locked=%d %s\n",
+             fCount, fTotalBytesUsed, locked,
+             fDiscardableFactory ? "discardable" : "malloc");
 }
 
 size_t SkResourceCache::setSingleAllocationByteLimit(size_t newLimit) {
@@ -414,6 +470,35 @@ static SkResourceCache* get_cache() {
     return gResourceCache;
 }
 
+void SkResourceCache::Unlock(SkResourceCache::ID id) {
+    SkAutoMutexAcquire am(gMutex);
+    get_cache()->unlock(id);
+
+//    get_cache()->dump();
+}
+
+void SkResourceCache::Remove(SkResourceCache::ID id) {
+    SkAutoMutexAcquire am(gMutex);
+    SkASSERT(id);
+
+#ifdef SK_DEBUG
+    {
+        bool found = false;
+        Rec* rec = get_cache()->fHead;
+        while (rec != NULL) {
+            if (rec == id) {
+                found = true;
+                break;
+            }
+            rec = rec->fNext;
+        }
+        SkASSERT(found);
+    }
+#endif
+    const Rec* rec = id;
+    get_cache()->remove(const_cast<Rec*>(rec));
+}
+
 size_t SkResourceCache::GetTotalBytesUsed() {
     SkAutoMutexAcquire am(gMutex);
     return get_cache()->getTotalBytesUsed();
@@ -454,9 +539,14 @@ void SkResourceCache::PurgeAll() {
     return get_cache()->purgeAll();
 }
 
-bool SkResourceCache::Find(const Key& key, VisitorProc visitor, void* context) {
+const SkResourceCache::Rec* SkResourceCache::FindAndLock(const Key& key) {
+    SkAutoMutexAcquire am(gMutex);
+    return get_cache()->findAndLock(key);
+}
+
+const SkResourceCache::Rec* SkResourceCache::AddAndLock(Rec* rec) {
     SkAutoMutexAcquire am(gMutex);
-    return get_cache()->find(key, visitor, context);
+    return get_cache()->addAndLock(rec);
 }
 
 void SkResourceCache::Add(Rec* rec) {
index d4e1dfa3766fb74a94181ef5ff01092283a84107..dacd62cedb914af54e07b7b805d7d6a9e243d970 100644 (file)
@@ -60,7 +60,7 @@ public:
     struct Rec {
         typedef SkResourceCache::Key Key;
 
-        Rec() {}
+        Rec() : fLockCount(1) {}
         virtual ~Rec() {}
 
         uint32_t getHash() const { return this->getKey().hash(); }
@@ -75,24 +75,13 @@ public:
     private:
         Rec*    fNext;
         Rec*    fPrev;
+        int32_t fLockCount;
 
         friend class SkResourceCache;
     };
 
     typedef const Rec* ID;
 
-    /**
-     *  Callback function for find(). If called, the cache will have found a match for the
-     *  specified Key, and will pass in the corresponding Rec, along with a caller-specified
-     *  context. The function can read the data in Rec, and copy whatever it likes into context
-     *  (casting context to whatever it really is).
-     *
-     *  The return value determines what the cache will do with the Rec. If the function returns
-     *  true, then the Rec is considered "valid". If false is returned, the Rec will be considered
-     *  "stale" and will be purged from the cache.
-     */
-    typedef bool (*VisitorProc)(const Rec&, void* context);
-
     /**
      *  Returns a locked/pinned SkDiscardableMemory instance for the specified
      *  number of bytes, or NULL on failure.
@@ -104,17 +93,11 @@ public:
      *  instance of this cache.
      */
 
-    /**
-     *  Returns true if the visitor was called on a matching Key, and the visitor returned true.
-     *
-     *  Find() will search the cache for the specified Key. If no match is found, return false and
-     *  do not call the VisitorProc. If a match is found, return whatever the visitor returns.
-     *  Its return value is interpreted to mean:
-     *      true  : Rec is valid
-     *      false : Rec is "stale" -- the cache will purge it.
-     */
-    static bool Find(const Key& key, VisitorProc, void* context);
+    static const Rec* FindAndLock(const Key& key);
+    static const Rec* AddAndLock(Rec*);
     static void Add(Rec*);
+    static void Unlock(ID);
+    static void Remove(ID);
 
     static size_t GetTotalBytesUsed();
     static size_t GetTotalByteLimit();
@@ -156,17 +139,18 @@ public:
     explicit SkResourceCache(size_t byteLimit);
     ~SkResourceCache();
 
+    const Rec* findAndLock(const Key& key);
+    const Rec* addAndLock(Rec*);
+    void add(Rec*);
+    void remove(Rec*);
+
     /**
-     *  Returns true if the visitor was called on a matching Key, and the visitor returned true.
-     *
-     *  find() will search the cache for the specified Key. If no match is found, return false and
-     *  do not call the VisitorProc. If a match is found, return whatever the visitor returns.
-     *  Its return value is interpreted to mean:
-     *      true  : Rec is valid
-     *      false : Rec is "stale" -- the cache will purge it.
+     *  Given a non-null ID ptr returned by either findAndLock or addAndLock,
+     *  this releases the associated resources to be available to be purged
+     *  if needed. After this, the cached bitmap should no longer be
+     *  referenced by the caller.
      */
-    bool find(const Key&, VisitorProc, void* context);
-    void add(Rec*);
+    void unlock(ID);
 
     size_t getTotalBytesUsed() const { return fTotalBytesUsed; }
     size_t getTotalByteLimit() const { return fTotalByteLimit; }
@@ -218,7 +202,6 @@ private:
     void moveToHead(Rec*);
     void addToHead(Rec*);
     void detach(Rec*);
-    void remove(Rec*);
 
     void init();    // called by constructors
 
index 9f893bb24c48c6b06024497961475bd8d8e31059..317ed6da155cfc306f70a3e9c7ca0e0b9dbb2b03 100644 (file)
@@ -27,46 +27,49 @@ struct TestingRec : public SkResourceCache::Rec {
 
     virtual const Key& getKey() const SK_OVERRIDE { return fKey; }
     virtual size_t bytesUsed() const SK_OVERRIDE { return sizeof(fKey) + sizeof(fValue); }
-
-    static bool Visitor(const SkResourceCache::Rec& baseRec, void* context) {
-        const TestingRec& rec = static_cast<const TestingRec&>(baseRec);
-        intptr_t* result = (intptr_t*)context;
-        
-        *result = rec.fValue;
-        return true;
-    }
 };
 }
 
 static const int COUNT = 10;
 static const int DIM = 256;
 
-static void test_cache(skiatest::Reporter* reporter, SkResourceCache& cache, bool testPurge) {
+static void test_cache(skiatest::Reporter* reporter, SkResourceCache& cache,
+                       bool testPurge) {
+    SkResourceCache::ID id;
+
     for (int i = 0; i < COUNT; ++i) {
         TestingKey key(i);
-        intptr_t value = -1;
 
-        REPORTER_ASSERT(reporter, !cache.find(key, TestingRec::Visitor, &value));
-        REPORTER_ASSERT(reporter, -1 == value);
+        const TestingRec* rec = (const TestingRec*)cache.findAndLock(key);
+        REPORTER_ASSERT(reporter, NULL == rec);
 
-        cache.add(SkNEW_ARGS(TestingRec, (key, i)));
+        TestingRec* newRec = SkNEW_ARGS(TestingRec, (key, i));
+        const TestingRec* addedRec = (const TestingRec*)cache.addAndLock(newRec);
+        REPORTER_ASSERT(reporter, addedRec);
 
-        REPORTER_ASSERT(reporter, cache.find(key, TestingRec::Visitor, &value));
-        REPORTER_ASSERT(reporter, i == value);
+        const TestingRec* foundRec = (const TestingRec*)cache.findAndLock(key);
+        REPORTER_ASSERT(reporter, foundRec == addedRec);
+        REPORTER_ASSERT(reporter, foundRec->fValue == i);
+        cache.unlock(foundRec);
+        cache.unlock(addedRec);
     }
 
     if (testPurge) {
         // stress test, should trigger purges
         for (size_t i = 0; i < COUNT * 100; ++i) {
             TestingKey key(i);
-            cache.add(SkNEW_ARGS(TestingRec, (key, i)));
+            SkResourceCache::ID id = cache.addAndLock(SkNEW_ARGS(TestingRec, (key, i)));
+            REPORTER_ASSERT(reporter, id);
+            cache.unlock(id);
         }
     }
 
     // test the originals after all that purging
     for (int i = 0; i < COUNT; ++i) {
-        intptr_t value;
-        (void)cache.find(TestingKey(i), TestingRec::Visitor, &value);
+        id = cache.findAndLock(TestingKey(i));
+        if (id) {
+            cache.unlock(id);
+        }
     }
 
     cache.setTotalByteLimit(0);
@@ -106,11 +109,15 @@ DEF_TEST(ImageCache_doubleAdd, r) {
 
     TestingKey key(1);
 
-    cache.add(SkNEW_ARGS(TestingRec, (key, 2)));
-    cache.add(SkNEW_ARGS(TestingRec, (key, 3)));
+    SkResourceCache::ID id1 = cache.addAndLock(SkNEW_ARGS(TestingRec, (key, 2)));
+    SkResourceCache::ID id2 = cache.addAndLock(SkNEW_ARGS(TestingRec, (key, 3)));
+    // We don't really care if id1 == id2 as long as unlocking both works.
+    cache.unlock(id1);
+    cache.unlock(id2);
 
     // Lookup can return either value.
-    intptr_t value = -1;
-    REPORTER_ASSERT(r, cache.find(key, TestingRec::Visitor, &value));
-    REPORTER_ASSERT(r, 2 == value || 3 == value);
+    const TestingRec* rec = (const TestingRec*)cache.findAndLock(key);
+    REPORTER_ASSERT(r, rec);
+    REPORTER_ASSERT(r, 2 == rec->fValue || 3 == rec->fValue);
+    cache.unlock(rec);
 }