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

This commit is contained in:
Chris Kitching 2014-03-12 16:20:36 +00:00
parent ce76f17105
commit 6269828bc6
2 changed files with 66 additions and 106 deletions

View File

@ -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();
}

View File

@ -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;