From dee6a8e67db39fcbde2b3bb09be1d088ebb9db8a Mon Sep 17 00:00:00 2001 From: reed Date: Mon, 15 Sep 2014 06:44:47 -0700 Subject: [PATCH] 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: R=mtklein@google.com, danakj@chromium.org Author: reed@google.com Review URL: https://codereview.chromium.org/567393002 --- bench/ImageCacheBench.cpp | 10 ++- src/core/SkBitmapCache.cpp | 43 ++++++------- src/core/SkResourceCache.cpp | 120 +++++------------------------------ src/core/SkResourceCache.h | 49 +++++++++----- tests/ImageCacheTest.cpp | 53 +++++++--------- 5 files changed, 98 insertions(+), 177 deletions(-) diff --git a/bench/ImageCacheBench.cpp b/bench/ImageCacheBench.cpp index ca2b9342d5..0f8fdf2708 100644 --- a/bench/ImageCacheBench.cpp +++ b/bench/ImageCacheBench.cpp @@ -27,6 +27,10 @@ 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; + } }; } @@ -41,7 +45,7 @@ public: void populateCache() { for (int i = 0; i < CACHE_COUNT; ++i) { - fCache.unlock(fCache.addAndLock(SkNEW_ARGS(TestRec, (TestKey(i), i)))); + fCache.add(SkNEW_ARGS(TestRec, (TestKey(i), i))); } } @@ -58,8 +62,8 @@ protected: TestKey key(-1); // search for a miss (-1) for (int i = 0; i < loops; ++i) { - SkDEBUGCODE(SkResourceCache::ID id =) fCache.findAndLock(key); - SkASSERT(NULL == id); + SkDEBUGCODE(bool found =) fCache.find(key, TestRec::Visitor, NULL); + SkASSERT(!found); } } diff --git a/src/core/SkBitmapCache.cpp b/src/core/SkBitmapCache.cpp index 6d4f4b4cc7..84f10363f0 100644 --- a/src/core/SkBitmapCache.cpp +++ b/src/core/SkBitmapCache.cpp @@ -59,25 +59,16 @@ 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 find_and_return(const BitmapKey& key, SkBitmap* result) { - const BitmapRec* rec = (BitmapRec*)SkResourceCache::FindAndLock(key); - if (rec) { - *result = rec->fBitmap; - SkResourceCache::Unlock(rec); + static bool Visitor(const SkResourceCache::Rec& baseRec, void* contextBitmap) { + const BitmapRec& rec = static_cast(baseRec); + SkBitmap* result = (SkBitmap*)contextBitmap; + *result = rec.fBitmap; result->lockPixels(); - if (result->getPixels()) { - return true; - } - - SkResourceCache::Remove(rec); - result->reset(); - // fall-through to false + return SkToBool(result->getPixels()); } - return false; -} +}; bool SkBitmapCache::Find(const SkBitmap& src, SkScalar invScaleX, SkScalar invScaleY, SkBitmap* result) { @@ -86,7 +77,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 find_and_return(key, result); + return SkResourceCache::Find(key, BitmapRec::Visitor, result); } void SkBitmapCache::Add(const SkBitmap& src, SkScalar invScaleX, SkScalar invScaleY, @@ -102,7 +93,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 find_and_return(key, result); + return SkResourceCache::Find(key, BitmapRec::Visitor, result); } bool SkBitmapCache::Add(uint32_t genID, const SkIRect& subset, const SkBitmap& result) { @@ -138,16 +129,22 @@ 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(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 MipMapRec* rec = (MipMapRec*)SkResourceCache::FindAndLock(key); - const SkMipMap* result = NULL; - if (rec) { - result = SkRef(rec->fMipMap); - SkResourceCache::Unlock(rec); + const SkMipMap* result; + if (!SkResourceCache::Find(key, MipMapRec::Visitor, &result)) { + result = NULL; } return result; } diff --git a/src/core/SkResourceCache.cpp b/src/core/SkResourceCache.cpp index 68248f512b..85b5b907b1 100644 --- a/src/core/SkResourceCache.cpp +++ b/src/core/SkResourceCache.cpp @@ -187,80 +187,34 @@ SkResourceCache::~SkResourceCache() { //////////////////////////////////////////////////////////////////////////////// -const SkResourceCache::Rec* SkResourceCache::findAndLock(const Key& key) { +bool SkResourceCache::find(const Key& key, VisitorProc visitor, void* context) { Rec* rec = fHash->find(key); if (rec) { - this->moveToHead(rec); // for our LRU - rec->fLockCount += 1; + if (visitor(*rec, context)) { + this->moveToHead(rec); // for our LRU + return true; + } else { + this->remove(rec); // stale + 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; + return false; } void SkResourceCache::add(Rec* rec) { SkASSERT(rec); // See if we already have this key (racy inserts, etc.) - const Rec* existing = this->findAndLock(rec->getKey()); + Rec* existing = fHash->find(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)->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); @@ -292,9 +246,7 @@ void SkResourceCache::purgeAsNeeded(bool forcePurge) { } Rec* prev = rec->fPrev; - if (0 == rec->fLockCount) { - this->remove(rec); - } + this->remove(rec); rec = prev; } } @@ -417,16 +369,8 @@ void SkResourceCache::validate() const { void SkResourceCache::dump() const { this->validate(); - 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"); + SkDebugf("SkResourceCache: count=%d bytes=%d %s\n", + fCount, fTotalBytesUsed, fDiscardableFactory ? "discardable" : "malloc"); } size_t SkResourceCache::setSingleAllocationByteLimit(size_t newLimit) { @@ -470,35 +414,6 @@ 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)); -} - size_t SkResourceCache::GetTotalBytesUsed() { SkAutoMutexAcquire am(gMutex); return get_cache()->getTotalBytesUsed(); @@ -539,14 +454,9 @@ void SkResourceCache::PurgeAll() { return get_cache()->purgeAll(); } -const SkResourceCache::Rec* SkResourceCache::FindAndLock(const Key& key) { +bool SkResourceCache::Find(const Key& key, VisitorProc visitor, void* context) { SkAutoMutexAcquire am(gMutex); - return get_cache()->findAndLock(key); -} - -const SkResourceCache::Rec* SkResourceCache::AddAndLock(Rec* rec) { - SkAutoMutexAcquire am(gMutex); - return get_cache()->addAndLock(rec); + return get_cache()->find(key, visitor, context); } void SkResourceCache::Add(Rec* rec) { diff --git a/src/core/SkResourceCache.h b/src/core/SkResourceCache.h index dacd62cedb..d4e1dfa376 100644 --- a/src/core/SkResourceCache.h +++ b/src/core/SkResourceCache.h @@ -60,7 +60,7 @@ public: struct Rec { typedef SkResourceCache::Key Key; - Rec() : fLockCount(1) {} + Rec() {} virtual ~Rec() {} uint32_t getHash() const { return this->getKey().hash(); } @@ -75,13 +75,24 @@ 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. @@ -93,11 +104,17 @@ public: * instance of this cache. */ - static const Rec* FindAndLock(const Key& key); - static const Rec* AddAndLock(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. + */ + static bool Find(const Key& key, VisitorProc, void* context); static void Add(Rec*); - static void Unlock(ID); - static void Remove(ID); static size_t GetTotalBytesUsed(); static size_t GetTotalByteLimit(); @@ -139,18 +156,17 @@ public: explicit SkResourceCache(size_t byteLimit); ~SkResourceCache(); - const Rec* findAndLock(const Key& key); - const Rec* addAndLock(Rec*); - void add(Rec*); - void remove(Rec*); - /** - * 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. + * 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. */ - void unlock(ID); + bool find(const Key&, VisitorProc, void* context); + void add(Rec*); size_t getTotalBytesUsed() const { return fTotalBytesUsed; } size_t getTotalByteLimit() const { return fTotalByteLimit; } @@ -202,6 +218,7 @@ private: void moveToHead(Rec*); void addToHead(Rec*); void detach(Rec*); + void remove(Rec*); void init(); // called by constructors diff --git a/tests/ImageCacheTest.cpp b/tests/ImageCacheTest.cpp index 317ed6da15..9f893bb24c 100644 --- a/tests/ImageCacheTest.cpp +++ b/tests/ImageCacheTest.cpp @@ -27,49 +27,46 @@ 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(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) { - SkResourceCache::ID id; - +static void test_cache(skiatest::Reporter* reporter, SkResourceCache& cache, bool testPurge) { for (int i = 0; i < COUNT; ++i) { TestingKey key(i); + intptr_t value = -1; - const TestingRec* rec = (const TestingRec*)cache.findAndLock(key); - REPORTER_ASSERT(reporter, NULL == rec); + REPORTER_ASSERT(reporter, !cache.find(key, TestingRec::Visitor, &value)); + REPORTER_ASSERT(reporter, -1 == value); - TestingRec* newRec = SkNEW_ARGS(TestingRec, (key, i)); - const TestingRec* addedRec = (const TestingRec*)cache.addAndLock(newRec); - REPORTER_ASSERT(reporter, addedRec); + cache.add(SkNEW_ARGS(TestingRec, (key, i))); - 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); + REPORTER_ASSERT(reporter, cache.find(key, TestingRec::Visitor, &value)); + REPORTER_ASSERT(reporter, i == value); } if (testPurge) { // stress test, should trigger purges for (size_t i = 0; i < COUNT * 100; ++i) { TestingKey key(i); - SkResourceCache::ID id = cache.addAndLock(SkNEW_ARGS(TestingRec, (key, i))); - REPORTER_ASSERT(reporter, id); - cache.unlock(id); + cache.add(SkNEW_ARGS(TestingRec, (key, i))); } } // test the originals after all that purging for (int i = 0; i < COUNT; ++i) { - id = cache.findAndLock(TestingKey(i)); - if (id) { - cache.unlock(id); - } + intptr_t value; + (void)cache.find(TestingKey(i), TestingRec::Visitor, &value); } cache.setTotalByteLimit(0); @@ -109,15 +106,11 @@ DEF_TEST(ImageCache_doubleAdd, r) { TestingKey key(1); - 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); + cache.add(SkNEW_ARGS(TestingRec, (key, 2))); + cache.add(SkNEW_ARGS(TestingRec, (key, 3))); // Lookup can return either 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); + intptr_t value = -1; + REPORTER_ASSERT(r, cache.find(key, TestingRec::Visitor, &value)); + REPORTER_ASSERT(r, 2 == value || 3 == value); }