Skip to content

Commit

Permalink
Avoid obtaining size of recycled Bitmap in logs in BitmapPreFillRunner.
Browse files Browse the repository at this point in the history
If the BitmapPool is too full or the Bitmap is larger than the 
BitmapPool size, putting a Bitmap into the pool may recycle it. As a 
result it’s unsafe for us to use the Bitmap after the call to put() in 
the BitmapPool, including in logging. I’ve resolved the issue by 
memoizing the Bitmap size prior to the put() call.

Fixes #2574.
  • Loading branch information
sjudd committed Nov 27, 2017
1 parent 5ac08f9 commit 7387298
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
* limit, we assume a GC has occurred, stop the current allocations, and try again after a delay.
*/
final class BitmapPreFillRunner implements Runnable {
private static final String TAG = "PreFillRunner";
@VisibleForTesting
static final String TAG = "PreFillRunner";
private static final Clock DEFAULT_CLOCK = new Clock();

/**
Expand Down Expand Up @@ -65,15 +66,23 @@ final class BitmapPreFillRunner implements Runnable {

// Public API.
@SuppressWarnings("WeakerAccess")
public BitmapPreFillRunner(BitmapPool bitmapPool, MemoryCache memoryCache,
PreFillQueue allocationOrder) {
this(bitmapPool, memoryCache, allocationOrder, DEFAULT_CLOCK,
public BitmapPreFillRunner(
BitmapPool bitmapPool, MemoryCache memoryCache, PreFillQueue allocationOrder) {
this(
bitmapPool,
memoryCache,
allocationOrder,
DEFAULT_CLOCK,
new Handler(Looper.getMainLooper()));
}

@VisibleForTesting
BitmapPreFillRunner(BitmapPool bitmapPool, MemoryCache memoryCache, PreFillQueue allocationOrder,
Clock clock, Handler handler) {
BitmapPreFillRunner(
BitmapPool bitmapPool,
MemoryCache memoryCache,
PreFillQueue allocationOrder,
Clock clock,
Handler handler) {
this.bitmapPool = bitmapPool;
this.memoryCache = memoryCache;
this.toPrefill = allocationOrder;
Expand All @@ -89,33 +98,39 @@ public void cancel() {
* Attempts to allocate {@link android.graphics.Bitmap}s and returns {@code true} if there are
* more {@link android.graphics.Bitmap}s to allocate and {@code false} otherwise.
*/
private boolean allocate() {
@VisibleForTesting
boolean allocate() {
long start = clock.now();
while (!toPrefill.isEmpty() && !isGcDetected(start)) {
PreFillType toAllocate = toPrefill.remove();
final Bitmap bitmap;
if (!seenTypes.contains(toAllocate)) {
seenTypes.add(toAllocate);
bitmap = bitmapPool.getDirty(toAllocate.getWidth(), toAllocate.getHeight(),
toAllocate.getConfig());
bitmap =
bitmapPool.getDirty(
toAllocate.getWidth(), toAllocate.getHeight(), toAllocate.getConfig());
} else {
bitmap = Bitmap.createBitmap(toAllocate.getWidth(), toAllocate.getHeight(),
toAllocate.getConfig());
bitmap =
Bitmap.createBitmap(
toAllocate.getWidth(), toAllocate.getHeight(), toAllocate.getConfig());
}

// Order matters here! If the Bitmap is too large or the BitmapPool is too full, it may be
// recycled after the call to bitmapPool#put below.
int bitmapSize = Util.getBitmapByteSize(bitmap);

// Don't over fill the memory cache to avoid evicting useful resources, but make sure it's
// not empty so
// we use all available space.
if (getFreeMemoryCacheBytes() >= Util.getBitmapByteSize(bitmap)) {
// not empty so that we use all available space.
if (getFreeMemoryCacheBytes() >= bitmapSize) {
memoryCache.put(new UniqueKey(), BitmapResource.obtain(bitmap, bitmapPool));
} else {
bitmapPool.put(bitmap);
}

if (Log.isLoggable(TAG, Log.DEBUG)) {
Log.d(TAG,
"allocated [" + toAllocate.getWidth() + "x" + toAllocate.getHeight() + "] " + toAllocate
.getConfig() + " size: " + Util.getBitmapByteSize(bitmap));
"allocated [" + toAllocate.getWidth() + "x" + toAllocate.getHeight() + "] "
+ toAllocate.getConfig() + " size: " + bitmapSize);
}
}

Expand Down Expand Up @@ -143,7 +158,7 @@ private long getNextDelay() {
return result;
}

private static class UniqueKey implements Key {
private static final class UniqueKey implements Key {

@Synthetic
@SuppressWarnings("WeakerAccess")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,13 @@

import android.graphics.Bitmap;
import android.os.Handler;
import android.util.Log;
import com.bumptech.glide.load.Key;
import com.bumptech.glide.load.engine.Resource;
import com.bumptech.glide.load.engine.bitmap_recycle.BitmapPool;
import com.bumptech.glide.load.engine.bitmap_recycle.LruBitmapPool;
import com.bumptech.glide.load.engine.cache.MemoryCache;
import com.bumptech.glide.load.engine.cache.MemoryCacheAdapter;
import com.bumptech.glide.load.resource.bitmap.BitmapResource;
import com.bumptech.glide.tests.Util.CreateBitmap;
import com.bumptech.glide.util.Util;
Expand All @@ -37,6 +40,7 @@
import org.mockito.stubbing.Answer;
import org.robolectric.RobolectricTestRunner;
import org.robolectric.annotation.Config;
import org.robolectric.shadows.ShadowLog;

@RunWith(RobolectricTestRunner.class)
@Config(manifest = Config.NONE, sdk = 18)
Expand Down Expand Up @@ -300,6 +304,24 @@ public void testDoesNotGetMoreThanOncePerSize() {
// order.verify(pool, times(numBitmaps)).put(eq(bitmap));
}

@Test
public void allocate_whenBitmapPoolIsAtCapacity_doesNotLogWithRecycledBitmap() {
ShadowLog.setLoggable(BitmapPreFillRunner.TAG, Log.VERBOSE);

int dimensions = 10;
Bitmap.Config config = Bitmap.Config.ARGB_8888;
int bitmapByteSize = Util.getBitmapByteSize(dimensions, dimensions, config);
PreFillType preFillType = new PreFillType.Builder(dimensions).setConfig(config).build();
Map<PreFillType, Integer> allocationOrder = new HashMap<>();
allocationOrder.put(preFillType, 1);
PreFillQueue queue = new PreFillQueue(allocationOrder);
BitmapPreFillRunner runner =
new BitmapPreFillRunner(
new LruBitmapPool(bitmapByteSize - 1), new MemoryCacheAdapter(), queue);

runner.allocate();
}

private static final class AddBitmapPoolAnswer implements Answer<Void> {
private final List<Bitmap> bitmaps;

Expand Down

0 comments on commit 7387298

Please sign in to comment.