From cbd6b5d32aa5a805c6b68a006ac20ae4e594d777 Mon Sep 17 00:00:00 2001 From: Chris Kitching Date: Wed, 12 Mar 2014 16:20:36 +0000 Subject: [PATCH] Bug 969417 - Reduce insanity in favicon cache concurrency. r=rnewman --- .../base/favicons/cache/FaviconCache.java | 160 +++++++----------- .../base/favicons/cache/FaviconsForURL.java | 12 +- 2 files changed, 66 insertions(+), 106 deletions(-) diff --git a/mobile/android/base/favicons/cache/FaviconCache.java b/mobile/android/base/favicons/cache/FaviconCache.java index faa91e76d146..cbc0e7d2970b 100644 --- a/mobile/android/base/favicons/cache/FaviconCache.java +++ b/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(); } diff --git a/mobile/android/base/favicons/cache/FaviconsForURL.java b/mobile/android/base/favicons/cache/FaviconsForURL.java index 2e6400ed1cb5..9425738ad6f9 100644 --- a/mobile/android/base/favicons/cache/FaviconsForURL.java +++ b/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;