Refactor cache trimming

imagepipeline/src/main/java/com/facebook/imagepipeline/cache/BitmapMemoryCacheTrimStrategy.java

@@ -17,7 +17,7 @@
 /**
  * CountingMemoryCache eviction strategy appropriate for bitmap caches.
  *
- * <p>If run on KK or below, then this TrimStrategy behaves exactly as
+ * <p>If run on KitKat or below, then this TrimStrategy behaves exactly as
  * NativeMemoryCacheTrimStrategy. If run on Lollipop, then BitmapMemoryCacheTrimStrategy will trim
  * cache in one additional case: when OnCloseToDalvikHeapLimit trim type is received, cache's
  * eviction queue will be trimmed according to OnCloseToDalvikHeapLimit's suggested trim ratio.
@@ -26,24 +26,23 @@ public class BitmapMemoryCacheTrimStrategy implements CountingMemoryCache.CacheT
   private static final String TAG = "BitmapMemoryCacheTrimStrategy";
 
   @Override
-  public void trimCache(CountingMemoryCache<?, ?, ?> cache, MemoryTrimType trimType) {
+  public double getTrimRatio(MemoryTrimType trimType) {
     switch (trimType) {
       case OnCloseToDalvikHeapLimit:
         if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
-          final double cacheTrimTarget =
-              1 - MemoryTrimType.OnCloseToDalvikHeapLimit.getSuggestedTrimRatio();
-          final int cacheSizeTarget = (int) (cacheTrimTarget * cache.getSizeInBytes());
-          cache.trimCacheTo(Integer.MAX_VALUE, cacheSizeTarget);
+          return MemoryTrimType.OnCloseToDalvikHeapLimit.getSuggestedTrimRatio();
+        } else {
+          // On pre-lollipop versions we keep bitmaps on the native heap, so no need to trim here
+          // as it wouldn't help Dalvik heap anyway.
+          return 0;
         }
-        break;
       case OnAppBackgrounded:
       case OnSystemLowMemoryWhileAppInForeground:
       case OnSystemLowMemoryWhileAppInBackground:
-        cache.clearEvictionQueue();
-        break;
+        return 1;
       default:
         FLog.wtf(TAG, "unknown trim type: %s", trimType);
-        break;
+        return 0;
     }
   }
 }

imagepipeline/src/main/java/com/facebook/imagepipeline/cache/CountingMemoryCache.java

@@ -403,53 +403,41 @@ private List<CacheEntry<K, V>> getMatchingCachedEntries(Predicate<K> match) {
     return matchingEntries;
   }
 
-  /**
-   * Removes all exclusively owned values from the cache. Corresponding closeable references are
-   * closed.
-   */
-  public void clearEvictionQueue() {
-    Collection<CloseableReference<V>> evictedEntries = trimEvictionQueueTo(0, 0);
-    for (CloseableReference<V> reference : evictedEntries) {
-      reference.close();
-    }
-  }
-
-  public void trimCacheTo(int maxCount, int maxSize) {
+  @Override
+  public void trim(MemoryTrimType trimType) {
+    FLog.v(TAG, "Trimming cache, trim type %s", String.valueOf(trimType));
+    final double trimRatio = mCacheTrimStrategy.getTrimRatio(trimType);
     Collection<CloseableReference<V>> evictedEntries;
     synchronized (this) {
-      // Only max(maxCount - entries in use, 0) may stay in eviction queue
-      final int maxLruCount =
-          Math.max(maxCount - (mCachedEntries.size() - mEvictionQueue.size()), 0);
-      // Entries in eviction queue can occupy only max(maxSize - size of entries in use, 0) bytes
-      final int maxLruSize =
-          Math.max(maxSize - (mCachedValuesSize - mEvictionQueueSize), 0);
-      evictedEntries = trimEvictionQueueTo(maxLruCount, maxLruSize);
+      int targetCacheSize = (int) (mCachedValuesSize * (1 - trimRatio));
+      int targetEvictionQueueSize = Math.max(0, targetCacheSize - getInUseSizeInBytes());
+      evictedEntries = trimEvictionQueueTo(Integer.MAX_VALUE, targetEvictionQueueSize);
     }
     for (CloseableReference<V> reference : evictedEntries) {
       reference.close();
     }
   }
 
-  @Override
-  public void trim(MemoryTrimType trimType) {
-    FLog.v(TAG, "Trimming cache, trim type %s", String.valueOf(trimType));
-    mCacheTrimStrategy.trimCache(this, trimType);
-  }
-
-  /**
-   * @return number of cached entries
-   */
+  /** Gets the total number of currently cached entries. */
   public synchronized int getCount() {
     return mCachedEntries.size();
   }
 
-  /**
-   * @return total size in bytes of cached entries
-   */
+  /** Gets the total size in bytes of all currently cached entries. */
   public synchronized int getSizeInBytes() {
     return mCachedValuesSize;
   }
 
+  /** Gets the number of cached entries that are used by at least one client. */
+  public synchronized int getInUseCount() {
+    return mCachedEntries.size() - mEvictionQueue.size();
+  }
+
+  /** Gets the total size in bytes of cached entries that are used by at least one client. */
+  public synchronized int getInUseSizeInBytes() {
+    return mCachedValuesSize - mEvictionQueueSize;
+  }
+
   /**
    * @return number of cached entries eligible for eviction
    */
@@ -468,15 +456,10 @@ public synchronized int getEvictionQueueSizeInBytes() {
    * Check cache params to determine if the cache is capable of storing another value.
    */
   private synchronized boolean canCacheNewValue(final CloseableReference<V> value) {
-    Preconditions.checkState(mCachedValuesSize >= mEvictionQueueSize);
-    Preconditions.checkState(mCachedEntries.size() >= mEvictionQueue.size());
-
-    final long newValueSize = mValueInfoCallback.getSizeInBytes(value.get());
-    final long sharedEntries = mCachedEntries.size() - mEvictionQueue.size();
-    final long sharedEntriesByteSize = mCachedValuesSize - mEvictionQueueSize;
+    long newValueSize = mValueInfoCallback.getSizeInBytes(value.get());
     return (newValueSize <= mMemoryCacheParams.maxCacheEntrySize) &&
-        (sharedEntries < mMemoryCacheParams.maxCacheEntries) &&
-        (sharedEntriesByteSize + newValueSize <= mMemoryCacheParams.maxCacheSize);
+        (getInUseCount() + 1 <= mMemoryCacheParams.maxCacheEntries) &&
+        (getInUseSizeInBytes() + newValueSize <= mMemoryCacheParams.maxCacheSize);
   }
 
   /**
@@ -514,26 +497,17 @@ void maybeUpdateCacheParams() {
   @VisibleForTesting
   void maybeEvictEntries() {
     Collection<CloseableReference<V>> evictedValues;
-
     synchronized (this) {
-
-      final int allowedEvictionQueueCount = newEvictionQueueLimit(
-          mCachedEntries.size(),
-          mMemoryCacheParams.maxCacheEntries,
-          mEvictionQueue.size(),
-          mMemoryCacheParams.maxEvictionQueueEntries);
-
-      final long allowedEvictionQueueBytes = newEvictionQueueLimit(
-          mCachedValuesSize,
-          mMemoryCacheParams.maxCacheSize,
-          mEvictionQueueSize,
-          mMemoryCacheParams.maxEvictionQueueSize);
-
-      evictedValues = trimEvictionQueueTo(
-          allowedEvictionQueueCount,
-          allowedEvictionQueueBytes);
+      int maxCount = Math.min(
+          mMemoryCacheParams.maxEvictionQueueEntries,
+          mMemoryCacheParams.maxCacheEntries - getInUseCount());
+      maxCount = Math.max(maxCount, 0);
+      int maxSize = Math.min(
+          mMemoryCacheParams.maxEvictionQueueSize,
+          mMemoryCacheParams.maxCacheSize - getInUseSizeInBytes());
+      maxSize = Math.max(maxSize, 0);
+      evictedValues = trimEvictionQueueTo(maxCount, maxSize);
     }
-
     for (CloseableReference<V> evictedValue : evictedValues) {
       evictedValue.close();
     }
@@ -543,9 +517,7 @@ void maybeEvictEntries() {
    * Removes exclusively owned values from the cache until there is at most count of them
    * and they occupy no more than size bytes.
    */
-  private synchronized Collection<CloseableReference<V>> trimEvictionQueueTo(
-      int count,
-      long size) {
+  private synchronized Collection<CloseableReference<V>> trimEvictionQueueTo(int count, long size) {
     Preconditions.checkArgument(count >= 0);
     Preconditions.checkArgument(size >= 0);
 
@@ -674,19 +646,6 @@ private synchronized CloseableReference<V> handleIndexRegistration(
     }
   }
 
-  /**
-   * Helper method for computing eviction queue limit.
-   */
-  private static int newEvictionQueueLimit(
-      int currentTotal,
-      int maxTotal,
-      int currentEvictionQueue,
-      int maxEvictionQueue) {
-    final int trimNeeded = Math.max(0, currentTotal - maxTotal);
-    final int afterTrim = Math.max(0, currentEvictionQueue - trimNeeded);
-    return Math.min(maxEvictionQueue, afterTrim);
-  }
-
   /**
    * Interface used by the cache to query information about cached values.
    */
@@ -698,6 +657,6 @@ public static interface ValueInfoCallback<V> {
    * Interface used to specify cache trim behavior.
    */
   public static interface CacheTrimStrategy {
-    void trimCache(CountingMemoryCache<?, ?, ?> cache, MemoryTrimType trimType);
+    double getTrimRatio(MemoryTrimType trimType);
   }
 }

imagepipeline/src/main/java/com/facebook/imagepipeline/cache/NativeMemoryCacheTrimStrategy.java

@@ -13,7 +13,7 @@
 import com.facebook.common.memory.MemoryTrimType;
 
 /**
- * CountingMemoryCache eviction strategy appropriate for caches that store resources off the dalvik
+ * CountingMemoryCache eviction strategy appropriate for caches that store resources off the Dalvik
  * heap.
  *
  * <p>In case of OnCloseToDalvikHeapLimit nothing will be done. In case of other trim types
@@ -25,21 +25,18 @@ public class NativeMemoryCacheTrimStrategy implements CountingMemoryCache.CacheT
   public NativeMemoryCacheTrimStrategy() {}
 
   @Override
-  public void trimCache(CountingMemoryCache<?, ?, ?> cache, MemoryTrimType trimType) {
+  public double getTrimRatio(MemoryTrimType trimType) {
     switch (trimType) {
       case OnCloseToDalvikHeapLimit:
-        // Cached resources do not consume dalvik heap. We should not clear the
-        // cache in case of OnCloseToDalvikHeapLimit.
-        break;
+        // Resources cached on native heap do not consume Dalvik heap, so no trimming here.
+        return 0;
       case OnAppBackgrounded:
       case OnSystemLowMemoryWhileAppInForeground:
       case OnSystemLowMemoryWhileAppInBackground:
-        cache.clearEvictionQueue();
-        break;
+        return 1;
       default:
         FLog.wtf(TAG, "unknown trim type: %s", trimType);
-        break;
+        return 0;
     }
-
   }
 }