-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add resources that are dereferenced but not released to the cache.
Previously resources that were dereferenced but not released vanished into the ether. As a result you could load a Bitmap and then immediately after load the same Bitmap again and get a memory cache miss on the second load. This pattern is particularly common with the submit() APIs that load images on background threads where the Target is neither referenced by a view nor referenced directly. Fixes #2560
- Loading branch information
Showing
9 changed files
with
454 additions
and
183 deletions.
There are no files selected for viewing
158 changes: 158 additions & 0 deletions
158
instrumentation/src/androidTest/java/com/bumptech/glide/CachingTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,158 @@ | ||
package com.bumptech.glide; | ||
|
||
import static org.mockito.ArgumentMatchers.any; | ||
import static org.mockito.ArgumentMatchers.anyBoolean; | ||
import static org.mockito.ArgumentMatchers.eq; | ||
import static org.mockito.Mockito.verify; | ||
|
||
import android.content.Context; | ||
import android.graphics.Bitmap; | ||
import android.graphics.drawable.Drawable; | ||
import android.os.Handler; | ||
import android.os.Looper; | ||
import android.support.test.InstrumentationRegistry; | ||
import android.support.test.runner.AndroidJUnit4; | ||
import com.bumptech.glide.load.DataSource; | ||
import com.bumptech.glide.load.engine.DiskCacheStrategy; | ||
import com.bumptech.glide.load.engine.cache.LruResourceCache; | ||
import com.bumptech.glide.request.FutureTarget; | ||
import com.bumptech.glide.request.RequestListener; | ||
import com.bumptech.glide.request.target.Target; | ||
import com.bumptech.glide.test.BitmapSubject; | ||
import com.bumptech.glide.test.GlideApp; | ||
import com.bumptech.glide.test.ResourceIds; | ||
import com.bumptech.glide.test.TearDownGlide; | ||
import java.util.concurrent.CountDownLatch; | ||
import java.util.concurrent.ExecutionException; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.concurrent.TimeoutException; | ||
import org.junit.Before; | ||
import org.junit.Rule; | ||
import org.junit.Test; | ||
import org.junit.runner.RunWith; | ||
import org.mockito.Mock; | ||
import org.mockito.MockitoAnnotations; | ||
|
||
/** | ||
* Tests various aspects of memory and disk caching to verify resources can be retrieved as we | ||
* expect. | ||
*/ | ||
@RunWith(AndroidJUnit4.class) | ||
public class CachingTest { | ||
private static final int IMAGE_SIZE_PIXELS = 500; | ||
private static final long TIMEOUT_MS = 500; | ||
// Store at least 10 500x500 pixel Bitmaps with the ARGB_8888 config to be safe. | ||
private static final long CACHE_SIZE_BYTES = | ||
IMAGE_SIZE_PIXELS * IMAGE_SIZE_PIXELS * 4 * 10; | ||
|
||
@Rule public TearDownGlide tearDownGlide = new TearDownGlide(); | ||
@Mock private RequestListener<Drawable> requestListener; | ||
|
||
private Context context; | ||
|
||
@Before | ||
public void setUp() throws InterruptedException { | ||
MockitoAnnotations.initMocks(this); | ||
context = InstrumentationRegistry.getTargetContext(); | ||
|
||
Glide.init( | ||
context, new GlideBuilder().setMemoryCache(new LruResourceCache(CACHE_SIZE_BYTES))); | ||
} | ||
|
||
@Test | ||
public void submit_withPreviousRequestClearedFromMemory_completesFromDataDiskCache() | ||
throws InterruptedException, ExecutionException, TimeoutException { | ||
FutureTarget<Drawable> future = GlideApp.with(context) | ||
.load(ResourceIds.raw.canonical) | ||
.diskCacheStrategy(DiskCacheStrategy.DATA) | ||
.submit(IMAGE_SIZE_PIXELS, IMAGE_SIZE_PIXELS); | ||
future.get(); | ||
GlideApp.with(context).clear(future); | ||
|
||
clearMemoryCacheOnMainThread(); | ||
|
||
GlideApp.with(context) | ||
.load(ResourceIds.raw.canonical) | ||
.diskCacheStrategy(DiskCacheStrategy.DATA) | ||
.listener(requestListener) | ||
.submit(IMAGE_SIZE_PIXELS, IMAGE_SIZE_PIXELS) | ||
.get(); | ||
|
||
verify(requestListener) | ||
.onResourceReady( | ||
any(Drawable.class), | ||
any(), | ||
anyTarget(), | ||
eq(DataSource.DATA_DISK_CACHE), | ||
anyBoolean()); | ||
} | ||
|
||
@Test | ||
public void submit_withPreviousButNoLongerReferencedIdenticalRequest_completesFromMemoryCache() | ||
throws InterruptedException, TimeoutException, ExecutionException { | ||
// We can't allow any mocks (RequestListner, Target etc) to reference this request or the test | ||
// will fail due to the transient strong reference to the request. | ||
GlideApp.with(context) | ||
.load(ResourceIds.raw.canonical) | ||
.diskCacheStrategy(DiskCacheStrategy.RESOURCE) | ||
.submit(IMAGE_SIZE_PIXELS, IMAGE_SIZE_PIXELS) | ||
.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); | ||
// Force the collection of weak references now that the listener/request in the first load is no | ||
// longer referenced. | ||
Runtime.getRuntime().gc(); | ||
GlideApp.with(context) | ||
.load(ResourceIds.raw.canonical) | ||
.diskCacheStrategy(DiskCacheStrategy.RESOURCE) | ||
.listener(requestListener) | ||
.submit(IMAGE_SIZE_PIXELS, IMAGE_SIZE_PIXELS) | ||
.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); | ||
|
||
verify(requestListener).onResourceReady( | ||
any(Drawable.class), any(), anyTarget(), eq(DataSource.MEMORY_CACHE), anyBoolean()); | ||
} | ||
|
||
@Test | ||
public void submit_withPreviousButNoLongerReferencedIdenticalRequest_doesNotRecycleBitmap() | ||
throws InterruptedException, TimeoutException, ExecutionException { | ||
// We can't allow any mocks (RequestListener, Target etc) to reference this request or the test | ||
// will fail due to the transient strong reference to the request. | ||
Bitmap bitmap = GlideApp.with(context) | ||
.asBitmap() | ||
.load(ResourceIds.raw.canonical) | ||
.diskCacheStrategy(DiskCacheStrategy.RESOURCE) | ||
.submit(IMAGE_SIZE_PIXELS, IMAGE_SIZE_PIXELS) | ||
.get(); | ||
// Force the collection of weak references now that the listener/request in the first load is no | ||
// longer referenced. | ||
Runtime.getRuntime().gc(); | ||
|
||
FutureTarget<Bitmap> future = GlideApp.with(context) | ||
.asBitmap() | ||
.load(ResourceIds.raw.canonical) | ||
.diskCacheStrategy(DiskCacheStrategy.RESOURCE) | ||
.submit(IMAGE_SIZE_PIXELS, IMAGE_SIZE_PIXELS); | ||
future.get(); | ||
Glide.with(context).clear(future); | ||
|
||
clearMemoryCacheOnMainThread(); | ||
|
||
BitmapSubject.assertThat(bitmap).isNotRecycled(); | ||
} | ||
|
||
@SuppressWarnings("unchecked") | ||
private static Target<Drawable> anyTarget() { | ||
return (Target<Drawable>) any(Target.class); | ||
} | ||
|
||
private void clearMemoryCacheOnMainThread() throws InterruptedException { | ||
final CountDownLatch countDownLatch = new CountDownLatch(1); | ||
new Handler(Looper.getMainLooper()).post(new Runnable() { | ||
@Override | ||
public void run() { | ||
Glide.get(context).clearMemory(); | ||
countDownLatch.countDown(); | ||
} | ||
}); | ||
countDownLatch.await(); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
97 changes: 97 additions & 0 deletions
97
library/src/main/java/com/bumptech/glide/load/engine/ActiveResources.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
package com.bumptech.glide.load.engine; | ||
|
||
import android.os.Looper; | ||
import android.os.MessageQueue; | ||
import android.support.annotation.NonNull; | ||
import android.support.annotation.Nullable; | ||
import com.bumptech.glide.load.Key; | ||
import com.bumptech.glide.load.engine.EngineResource.ResourceListener; | ||
import com.bumptech.glide.util.Preconditions; | ||
import com.bumptech.glide.util.Synthetic; | ||
import java.lang.ref.ReferenceQueue; | ||
import java.lang.ref.WeakReference; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
final class ActiveResources { | ||
private final Map<Key, ResourceWeakReference> activeEngineResources = new HashMap<>(); | ||
// Lazily instantiate to avoid exceptions if Glide is initialized on a background thread. See | ||
// #295. | ||
@Nullable | ||
private ReferenceQueue<EngineResource<?>> resourceReferenceQueue; | ||
private ResourceListener listener; | ||
|
||
void setListener(ResourceListener listener) { | ||
this.listener = listener; | ||
} | ||
|
||
void activate(Key key, EngineResource<?> resource) { | ||
activeEngineResources.put(key, new ResourceWeakReference(key, resource, getReferenceQueue())); | ||
} | ||
|
||
void deactivate(Key key) { | ||
activeEngineResources.remove(key); | ||
} | ||
|
||
@Nullable | ||
EngineResource<?> get(Key key) { | ||
ResourceWeakReference activeRef = activeEngineResources.get(key); | ||
if (activeRef == null) { | ||
return null; | ||
} | ||
|
||
EngineResource<?> active = activeRef.get(); | ||
if (active == null) { | ||
cleanupActiveReference(activeRef); | ||
} | ||
return active; | ||
} | ||
|
||
private void cleanupActiveReference(@NonNull ResourceWeakReference ref) { | ||
activeEngineResources.remove(ref.key); | ||
|
||
if (!ref.isCacheable) { | ||
return; | ||
} | ||
EngineResource<?> newResource = | ||
new EngineResource<>(ref.resource, /*isCacheable=*/ true, /*isRecyclable=*/ false); | ||
newResource.setResourceListener(ref.key, listener); | ||
listener.onResourceReleased(ref.key, newResource); | ||
} | ||
|
||
private ReferenceQueue<EngineResource<?>> getReferenceQueue() { | ||
if (resourceReferenceQueue == null) { | ||
resourceReferenceQueue = new ReferenceQueue<>(); | ||
MessageQueue queue = Looper.myQueue(); | ||
queue.addIdleHandler(new RefQueueIdleHandler()); | ||
} | ||
return resourceReferenceQueue; | ||
} | ||
|
||
// Responsible for cleaning up the active resource map by remove weak references that have been | ||
// cleared. | ||
private class RefQueueIdleHandler implements MessageQueue.IdleHandler { | ||
@Override | ||
public boolean queueIdle() { | ||
ResourceWeakReference ref; | ||
while ((ref = (ResourceWeakReference) getReferenceQueue().poll()) != null) { | ||
cleanupActiveReference(ref); | ||
} | ||
return true; | ||
} | ||
} | ||
|
||
private static class ResourceWeakReference extends WeakReference<EngineResource<?>> { | ||
@Synthetic final Key key; | ||
@Synthetic final EngineResource<?> resource; | ||
@Synthetic final boolean isCacheable; | ||
|
||
ResourceWeakReference( | ||
Key key, EngineResource<?> r, ReferenceQueue<? super EngineResource<?>> q) { | ||
super(r, q); | ||
this.key = Preconditions.checkNotNull(key); | ||
this.resource = Preconditions.checkNotNull(r); | ||
isCacheable = r.isCacheable(); | ||
} | ||
} | ||
} |
Oops, something went wrong.