Bug 969417 - Reduce insanity in favicon cache concurrency. r=rnewman

mobile/android/base/favicons/cache/FaviconCache.java

@@ -159,18 +159,6 @@ public class FaviconCache {
         }
     }
 
-    /**
-     * An alternative to startWrite to be used when in a read transaction and wanting to upgrade it
-     * to a write transaction. Such a transaction should be terminated with finishWrite.
-     */
-    private void upgradeReadToWrite() {
-        mTurnSemaphore.acquireUninterruptibly();
-        if (mOngoingReads.decrementAndGet() == 0) {
-            mWriteLock.release();
-        }
-        mWriteLock.acquireUninterruptibly();
-    }
-
     /**
      * Called by transactions performing only reads as they finish. Ensures that if this is the last
      * concluding read transaction then then writers are subsequently allowed in.
@@ -217,9 +205,6 @@ public class FaviconCache {
 
         startRead();
 
-        boolean isExpired = false;
-        boolean isAborting = false;
-
         try {
             // If we don't have it in the cache, it certainly isn't a known failure.
             // Non-evictable favicons are never failed, so we don't need to
@@ -240,34 +225,22 @@ public class FaviconCache {
             // Calculate elapsed time since the failing download.
             final long failureDiff = System.currentTimeMillis() - failureTimestamp;
 
-            // If long enough has passed, mark it as no longer a failure.
-            if (failureDiff > FAILURE_RETRY_MILLISECONDS) {
-                isExpired = true;
-            } else {
+            // If the expiry is still in effect, return. Otherwise, continue and unmark the failure.
+            if (failureDiff < FAILURE_RETRY_MILLISECONDS) {
                 return true;
             }
         } catch (Exception unhandled) {
-            // Handle any exception thrown and return the locks to a sensible state.
-            finishRead();
-
-            // Flag to prevent finally from doubly-unlocking.
-            isAborting = true;
             Log.e(LOGTAG, "FaviconCache exception!", unhandled);
             return true;
         }  finally {
-            if (!isAborting) {
-                if (isExpired) {
-                    // No longer expired.
-                    upgradeReadToWrite();
-                } else {
-                    finishRead();
-                }
-            }
+            finishRead();
         }
 
+        startWrite();
+
+        // If the entry is no longer failed, remove the record of it from the cache.
         try {
-            recordRemoved(mBackingMap.get(faviconURL));
-            mBackingMap.remove(faviconURL);
+            recordRemoved(mBackingMap.remove(faviconURL));
             return false;
         } finally {
             finishWrite();
@@ -282,14 +255,12 @@ public class FaviconCache {
     public void putFailed(String faviconURL) {
         startWrite();
 
-        if (mBackingMap.containsKey(faviconURL)) {
-            recordRemoved(mBackingMap.get(faviconURL));
+        try {
+            FaviconsForURL container = new FaviconsForURL(0, true);
+            recordRemoved(mBackingMap.put(faviconURL, container));
+        } finally {
+            finishWrite();
         }
-
-        FaviconsForURL container = new FaviconsForURL(0, true);
-        mBackingMap.put(faviconURL, container);
-
-        finishWrite();
     }
 
     /**
@@ -309,9 +280,7 @@ public class FaviconCache {
             return null;
         }
 
-        boolean doingWrites = false;
         boolean shouldComputeColour = false;
-        boolean isAborting = false;
         boolean wasPermanent = false;
         FaviconsForURL container;
         final Bitmap newBitmap;
@@ -347,7 +316,7 @@ public class FaviconCache {
 
                 // If we found exactly what we wanted - we're done.
                 if (cacheElement.mImageSize == targetSize) {
-                    setMostRecentlyUsed(cacheElement);
+                    setMostRecentlyUsedWithinRead(cacheElement);
                     return cacheElement.mFaviconPayload;
                 }
             } else {
@@ -373,11 +342,6 @@ public class FaviconCache {
                 return cacheElement.mFaviconPayload;
             }
 
-            // Having got this far, we'll be needing to write the new secondary to the cache, which
-            // involves us falling through to the next try block. This flag lets us do this (Other
-            // paths prior to this end in returns.)
-            doingWrites = true;
-
             // Scaling logic...
             Bitmap largestElementBitmap = cacheElement.mFaviconPayload;
             int largestSize = cacheElement.mImageSize;
@@ -401,24 +365,16 @@ public class FaviconCache {
                 }
             }
         } catch (Exception unhandled) {
-            isAborting = true;
-
             // Handle any exception thrown and return the locks to a sensible state.
-            finishRead();
 
             // Flag to prevent finally from doubly-unlocking.
             Log.e(LOGTAG, "FaviconCache exception!", unhandled);
             return null;
         } finally {
-            if (!isAborting) {
-                if (doingWrites) {
-                    upgradeReadToWrite();
-                } else {
-                    finishRead();
-                }
-            }
+            finishRead();
         }
 
+        startWrite();
         try {
             if (shouldComputeColour) {
                 // And since we failed, we'll need the dominant colour.
@@ -432,8 +388,9 @@ public class FaviconCache {
             FaviconCacheElement newElement = container.addSecondary(newBitmap, targetSize);
 
             if (!wasPermanent) {
-                setMostRecentlyUsed(newElement);
-                mCurrentSize.addAndGet(newElement.sizeOf());
+                if (setMostRecentlyUsedWithinWrite(newElement)) {
+                    mCurrentSize.addAndGet(newElement.sizeOf());
+                }
             }
         } finally {
             finishWrite();
@@ -464,7 +421,6 @@ public class FaviconCache {
                 return 0xFFFFFF;
             }
 
-
             return element.ensureDominantColor();
         } finally {
             finishRead();
@@ -472,8 +428,8 @@ public class FaviconCache {
     }
 
     /**
-     * Remove all payloads stored in the given container from the LRU cache. Must be called while
-     * holding the write lock.
+     * Remove all payloads stored in the given container from the LRU cache.
+     * Must be called while holding the write lock.
      *
      * @param wasRemoved The container to purge from the cache.
      */
@@ -505,20 +461,39 @@ public class FaviconCache {
         if (favicon.getWidth() > mMaxCachedWidth) {
             return Bitmap.createScaledBitmap(favicon, mMaxCachedWidth, mMaxCachedWidth, true);
         }
+
         return favicon;
     }
 
     /**
-     * Set an existing element as the most recently used element. May be called from either type of
-     * transaction.
+     * Set an existing element as the most recently used element. Intended for use from read transactions. While
+     * write transactions may safely use this method, it will perform slightly worse than its unsafe counterpart below.
      *
      * @param element The element that is to become the most recently used one.
+     * @return true if this element already existed in the list, false otherwise. (Useful for preventing multiple-insertion.)
      */
-    private void setMostRecentlyUsed(FaviconCacheElement element) {
+    private boolean setMostRecentlyUsedWithinRead(FaviconCacheElement element) {
         mReorderingSemaphore.acquireUninterruptibly();
-        mOrdering.remove(element);
+        try {
+            boolean contained = mOrdering.remove(element);
+            mOrdering.offer(element);
+            return contained;
+        } finally {
+            mReorderingSemaphore.release();
+        }
+    }
+
+    /**
+     * Functionally equivalent to setMostRecentlyUsedWithinRead, but operates without taking the reordering semaphore.
+     * Only safe for use when called from a write transaction, or there is a risk of concurrent modification.
+     *
+     * @param element The element that is to become the most recently used one.
+     * @return true if this element already existed in the list, false otherwise. (Useful for preventing multiple-insertion.)
+     */
+    private boolean setMostRecentlyUsedWithinWrite(FaviconCacheElement element) {
+        boolean contained = mOrdering.remove(element);
         mOrdering.offer(element);
-        mReorderingSemaphore.release();
+        return contained;
     }
 
     /**
@@ -546,7 +521,7 @@ public class FaviconCache {
         startWrite();
         try {
             // Set the new element as the most recently used one.
-            setMostRecentlyUsed(newElement);
+            setMostRecentlyUsedWithinWrite(newElement);
 
             mCurrentSize.addAndGet(newElement.sizeOf());
 
@@ -584,42 +559,23 @@ public class FaviconCache {
             sizeGained += newElement.sizeOf();
         }
 
-        startRead();
-
-        boolean abortingRead = false;
-
-        // Not using setMostRecentlyUsed, because the elements are known to be new. This can be done
-        // without taking the write lock, via the magic of the reordering semaphore.
-        mReorderingSemaphore.acquireUninterruptibly();
-        try {
-            if (!permanently) {
-                for (FaviconCacheElement newElement : toInsert.mFavicons) {
-                    mOrdering.offer(newElement);
-                }
-            }
-        } catch (Exception e) {
-            abortingRead = true;
-            mReorderingSemaphore.release();
-            finishRead();
-
-            Log.e(LOGTAG, "Favicon cache exception!", e);
-            return;
-        } finally {
-            if (!abortingRead) {
-                mReorderingSemaphore.release();
-                upgradeReadToWrite();
-            }
-        }
-
+        startWrite();
         try {
             if (permanently) {
                 mPermanentBackingMap.put(faviconURL, toInsert);
-            } else {
-                mCurrentSize.addAndGet(sizeGained);
-
-                // Update the value in the LruCache...
-                recordRemoved(mBackingMap.put(faviconURL, toInsert));
+                return;
             }
+
+            for (FaviconCacheElement newElement : toInsert.mFavicons) {
+                setMostRecentlyUsedWithinWrite(newElement);
+            }
+
+            // In the event this insertion is being made to a key that already held a value, the subsequent recordRemoved
+            // call will subtract the size of the old value, preventing double-counting.
+            mCurrentSize.addAndGet(sizeGained);
+
+            // Update the value in the LruCache...
+            recordRemoved(mBackingMap.put(faviconURL, toInsert));
         } finally {
             finishWrite();
         }

mobile/android/base/favicons/cache/FaviconsForURL.java

@@ -44,12 +44,16 @@ public class FaviconsForURL {
 
         int index = Collections.binarySearch(mFavicons, c);
 
+        // We've already got an equivalent one. We don't care about this new one. This only occurs in certain obscure
+        // case conditions.
+        if (index >= 0) {
+            return mFavicons.get(index);
+        }
+
         // binarySearch returns -x - 1 where x is the insertion point of the element. Convert
         // this to the actual insertion point..
-        if (index < 0) {
-            index++;
-            index = -index;
-        }
+        index++;
+        index = -index;
         mFavicons.add(index, c);
 
         return c;