Change SkResourceCache to take a Visitor inside its find().

src/core/SkResourceCache.cpp

Index: src/core/SkResourceCache.cpp
diff --git a/src/core/SkResourceCache.cpp b/src/core/SkResourceCache.cpp
index 68248f512bd2f08e4575e06d916e462750578d1f..85b5b907b11f08e705af4af0b0cd82451922cfb4 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;
-    }
-    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;
+        if (visitor(*rec, context)) {
+            this->moveToHead(rec);  // for our LRU
+            return true;
+        } else {
+            this->remove(rec);  // stale
+            return false;
+        }
     }
-
-    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*>(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();

[reed1, 2014/09/15 18:15:41]

The bug in the new CL was that we forgot to "purge" after the Add -- hence we
could overrun our budget!

[danakj, 2014/09/15 18:21:16]

Where is the purge in this CL then? I don't see one being added anywhere.

[reed1, 2014/09/15 18:22:49]

Just wanted to post the original that I'm reverting.
patch #2 has the purge-fix.

-    }
 }
 
 void SkResourceCache::remove(Rec* rec) {
-    SkASSERT(0 == rec->fLockCount);

[danakj, 2014/09/15 18:21:17]

This seems reasonable to keep, while the fLockCount still exists?

-
     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*>(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) {
-    SkAutoMutexAcquire am(gMutex);
-    return get_cache()->findAndLock(key);
-}
-
-const SkResourceCache::Rec* SkResourceCache::AddAndLock(Rec* rec) {
+bool SkResourceCache::Find(const Key& key, VisitorProc visitor, void* context) {
     SkAutoMutexAcquire am(gMutex);
-    return get_cache()->addAndLock(rec);
+    return get_cache()->find(key, visitor, context);
 }
 
 void SkResourceCache::Add(Rec* rec) {