From 5090b6d004fabbc0ae50e0295fd50e2f13cb6357 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Wed, 25 Mar 2020 17:47:01 -0700 Subject: [PATCH] Use a fixed size for items in LruCache. PiperOrigin-RevId: 303010044 --- .../com/bumptech/glide/util/LruCache.java | 59 ++++++++++++++----- .../glide/load/engine/cache/LruCacheTest.java | 55 ++++++++++++++++- 2 files changed, 97 insertions(+), 17 deletions(-) diff --git a/library/src/main/java/com/bumptech/glide/util/LruCache.java b/library/src/main/java/com/bumptech/glide/util/LruCache.java index 2cffcf4b6c..d171c3ba7f 100644 --- a/library/src/main/java/com/bumptech/glide/util/LruCache.java +++ b/library/src/main/java/com/bumptech/glide/util/LruCache.java @@ -15,7 +15,7 @@ * @param The type of the values. */ public class LruCache { - private final Map cache = new LinkedHashMap<>(100, 0.75f, true); + private final Map> cache = new LinkedHashMap<>(100, 0.75f, true); private final long initialMaxSize; private long maxSize; private long currentSize; @@ -98,7 +98,8 @@ public synchronized boolean contains(@NonNull T key) { */ @Nullable public synchronized Y get(@NonNull T key) { - return cache.get(key); + Entry entry = cache.get(key); + return entry != null ? entry.value : null; } /** @@ -109,6 +110,19 @@ public synchronized Y get(@NonNull T key) { * the cache and instead {@link #onItemEvicted(Object, Object)} will be called synchronously with * the given key and item. * + *

The size of the item is determined by the {@link #getSize(Object)} method. To avoid errors + * where {@link #getSize(Object)} returns different values for the same object when called at + * different times, the size value is acquired in {@code put} and retained until the item is + * evicted, replaced or removed. + * + *

If {@code item} is null the behavior here is a little odd. For the most part it's similar to + * simply calling {@link #remove(Object)} with the given key. The difference is that calling this + * method with a null {@code item} will result in an entry remaining in the cache with a null + * value and 0 size. The only real consequence is that at some point {@link #onItemEvicted(Object, + * Object)} may be called with the given {@code key} and a null value. Ideally we'd make calling + * this method with a null {@code item} identical to {@link #remove(Object)} but we're preserving + * this odd behavior to match older versions :(. + * * @param key The key to add the item at. * @param item The item to add. */ @@ -123,17 +137,17 @@ public synchronized Y put(@NonNull T key, @Nullable Y item) { if (item != null) { currentSize += itemSize; } - @Nullable final Y old = cache.put(key, item); + @Nullable Entry old = cache.put(key, item == null ? null : new Entry<>(item, itemSize)); if (old != null) { - currentSize -= getSize(old); + currentSize -= old.size; - if (!old.equals(item)) { - onItemEvicted(key, old); + if (!old.value.equals(item)) { + onItemEvicted(key, old.value); } } evict(); - return old; + return old != null ? old.value : null; } /** @@ -143,11 +157,12 @@ public synchronized Y put(@NonNull T key, @Nullable Y item) { */ @Nullable public synchronized Y remove(@NonNull T key) { - final Y value = cache.remove(key); - if (value != null) { - currentSize -= getSize(value); + Entry entry = cache.remove(key); + if (entry == null) { + return null; } - return value; + currentSize -= entry.size; + return entry.value; } /** Clears all items in the cache. */ @@ -162,20 +177,32 @@ public void clearMemory() { * @param size The size the cache should be less than. */ protected synchronized void trimToSize(long size) { - Map.Entry last; - Iterator> cacheIterator; + Map.Entry> last; + Iterator>> cacheIterator; while (currentSize > size) { cacheIterator = cache.entrySet().iterator(); last = cacheIterator.next(); - final Y toRemove = last.getValue(); - currentSize -= getSize(toRemove); + final Entry toRemove = last.getValue(); + currentSize -= toRemove.size; final T key = last.getKey(); cacheIterator.remove(); - onItemEvicted(key, toRemove); + onItemEvicted(key, toRemove.value); } } private void evict() { trimToSize(maxSize); } + + @Synthetic + static final class Entry { + final Y value; + final int size; + + @Synthetic + Entry(Y value, int size) { + this.value = value; + this.size = size; + } + } } diff --git a/library/test/src/test/java/com/bumptech/glide/load/engine/cache/LruCacheTest.java b/library/test/src/test/java/com/bumptech/glide/load/engine/cache/LruCacheTest.java index 85f186fdaa..9d16afff3d 100644 --- a/library/test/src/test/java/com/bumptech/glide/load/engine/cache/LruCacheTest.java +++ b/library/test/src/test/java/com/bumptech/glide/load/engine/cache/LruCacheTest.java @@ -1,5 +1,6 @@ package com.bumptech.glide.load.engine.cache; +import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; @@ -199,7 +200,7 @@ public void testCanPutSameItemMultipleTimes() { } @Test - public void put_withSameValueTwice_doesNotEvictItems() { + public void put_withSameKeyAndValueTwice_doesNotEvictItems() { String key = getKey(); Object value = new Object(); cache.put(key, value); @@ -318,6 +319,58 @@ public void testGetMaxSizeReturnsCurrentMaxSizeOfCache() { assertEquals(SIZE, cache.getMaxSize()); } + @Test + public void setSizeMultiplier_withItemWhoseSizeDecreasesAfterAdd_doesNotCrash() { + Object itemWhoseSizeWillChange = new Object(); + when(listener.getSize(itemWhoseSizeWillChange)).thenReturn(SIZE - 1).thenReturn(SIZE / 2); + cache.put(getKey(), itemWhoseSizeWillChange); + cache.setSizeMultiplier(0); + } + + @Test + public void getCurrentSize_afterRemovingItemWhoseSizeChanged_returnsZero() { + Object itemWhoseSizeWillChange = new Object(); + when(listener.getSize(itemWhoseSizeWillChange)).thenReturn(SIZE - 1).thenReturn(SIZE / 2); + String key = getKey(); + cache.put(key, itemWhoseSizeWillChange); + cache.remove(key); + + assertThat(cache.getCurrentSize()).isEqualTo(0); + } + + @Test + public void clearMemory_afterRemovingItemWhoseSizeChanged_doesNotCrash() { + Object itemWhoseSizeWillChange = new Object(); + when(listener.getSize(itemWhoseSizeWillChange)).thenReturn(SIZE - 1).thenReturn((SIZE / 2) - 1); + String key = getKey(); + cache.put(key, itemWhoseSizeWillChange); + cache.remove(key); + + cache.clearMemory(); + } + + @Test + public void getCurrentSize_afterUpdatingItemWhoseSizeChanged_returnsTheNewSize() { + Object itemWhoseSizeWillChange = new Object(); + when(listener.getSize(itemWhoseSizeWillChange)).thenReturn(SIZE - 1).thenReturn((SIZE / 2) - 1); + String key = getKey(); + cache.put(key, itemWhoseSizeWillChange); + cache.put(key, new Object()); + + assertThat(cache.getCurrentSize()).isEqualTo(1); + } + + @Test + public void clearMemory_afterUpdatingItemWhoseSizeChanged_doesNotCrash() { + Object itemWhoseSizeWillChange = new Object(); + when(listener.getSize(itemWhoseSizeWillChange)).thenReturn(SIZE - 1).thenReturn((SIZE / 2) - 1); + String key = getKey(); + cache.put(key, itemWhoseSizeWillChange); + cache.put(key, new Object()); + + cache.clearMemory(); + } + @Test public void testGetMaxSizeChangesIfMaxSizeChanges() { int multiplier = 2;