Skip to content

Commit

Permalink
Avoid pooling/reusing SingleRequest objects.
Browse files Browse the repository at this point in the history 
~90% of devices are now running on Art and won't get any benefit from
this object pooling. It's very difficult to definitively determine when
it's safe to recycle an object when it's used by multiple threads. The
code simplification is worth the minor performance regression on dalvik
devices.

PiperOrigin-RevId: 263229952
  • Loading branch information
@sjudd @glide-copybara-robot
sjudd authored and glide-copybara-robot committed Aug 13, 2019
1 parent 8a094e9 commit bee6348
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 135 deletions.
12 changes: 2 additions & 10 deletions library/src/main/java/com/bumptech/glide/load/engine/EngineJob.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -383,20 +383,16 @@ public StateVerifier getVerifier() {
private class CallLoadFailed implements Runnable {

private final ResourceCallback cb;
private final Object lock;

CallLoadFailed(ResourceCallback cb) {
this.cb = cb;
// The lock may be reset if the callback is removed while our Runnable is pending, so memoize
// it here. We're assuming that the lock is never re-used.
this.lock = Preconditions.checkNotNull(cb.getLock());
}

@Override
public void run() {
// Make sure we always acquire the request lock, then the EngineJob lock to avoid deadlock
// (b/136032534).
synchronized (lock) {
synchronized (cb.getLock()) {
synchronized (EngineJob.this) {
if (cbs.contains(cb)) {
callCallbackOnLoadFailed(cb);
Expand All @@ -411,20 +407,16 @@ public void run() {
private class CallResourceReady implements Runnable {

private final ResourceCallback cb;
private final Object lock;

CallResourceReady(ResourceCallback cb) {
this.cb = cb;
// The lock may be reset if the callback is removed while our Runnable is pending, so memoize
// it here. We're assuming that the lock is never re-used.
this.lock = Preconditions.checkNotNull(cb.getLock());
}

@Override
public void run() {
// Make sure we always acquire the request lock, then the EngineJob lock to avoid deadlock
// (b/136032534).
synchronized (lock) {
synchronized (cb.getLock()) {
synchronized (EngineJob.this) {
if (cbs.contains(cb)) {
// Acquire for this particular callback.
Expand Down
174 changes: 49 additions & 125 deletions library/src/main/java/com/bumptech/glide/request/SingleRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import androidx.annotation.GuardedBy;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.core.util.Pools;
import com.bumptech.glide.GlideContext;
import com.bumptech.glide.Priority;
import com.bumptech.glide.load.DataSource;
Expand All @@ -21,10 +20,7 @@
import com.bumptech.glide.request.transition.Transition;
import com.bumptech.glide.request.transition.TransitionFactory;
import com.bumptech.glide.util.LogTime;
import com.bumptech.glide.util.Preconditions;
import com.bumptech.glide.util.Synthetic;
import com.bumptech.glide.util.Util;
import com.bumptech.glide.util.pool.FactoryPools;
import com.bumptech.glide.util.pool.StateVerifier;
import java.util.List;
import java.util.concurrent.Executor;
Expand All @@ -35,24 +31,12 @@
*
* @param <R> The type of the resource that will be transcoded from the loaded resource.
*/
@SuppressWarnings("SynchronizeOnNonFinalField")
public final class SingleRequest<R>
implements Request, SizeReadyCallback, ResourceCallback, FactoryPools.Poolable {
public final class SingleRequest<R> implements Request, SizeReadyCallback, ResourceCallback {
/** Tag for logging internal events, not generally suitable for public use. */
private static final String TAG = "Request";
/** Tag for logging externally useful events (request completion, timing etc). */
private static final String GLIDE_TAG = "Glide";

private static final Pools.Pool<SingleRequest<?>> POOL =
FactoryPools.threadSafe(
150,
new FactoryPools.Factory<SingleRequest<?>>() {
@Override
public SingleRequest<?> create() {
return new SingleRequest<Object>();
}
});

private static final boolean IS_VERBOSE_LOGGABLE = Log.isLoggable(TAG, Log.VERBOSE);

private enum Status {
Expand All @@ -76,52 +60,35 @@ private enum Status {
private final StateVerifier stateVerifier = StateVerifier.newInstance();

/* Variables mutated only when a request is initialized or returned to the object pool. */
private volatile Object requestLock;
private final Object requestLock;

@GuardedBy("requestLock")
@Nullable
private RequestListener<R> targetListener;
@Nullable private final RequestListener<R> targetListener;

@GuardedBy("requestLock")
private RequestCoordinator requestCoordinator;
private final RequestCoordinator requestCoordinator;

@GuardedBy("requestLock")
private Context context;
private final Context context;

@GuardedBy("requestLock")
private GlideContext glideContext;
private final GlideContext glideContext;

@GuardedBy("requestLock")
@Nullable
private Object model;
@Nullable private final Object model;

@GuardedBy("requestLock")
private Class<R> transcodeClass;
private final Class<R> transcodeClass;

@GuardedBy("requestLock")
private BaseRequestOptions<?> requestOptions;
private final BaseRequestOptions<?> requestOptions;

@GuardedBy("requestLock")
private int overrideWidth;
private final int overrideWidth;

@GuardedBy("requestLock")
private int overrideHeight;
private final int overrideHeight;

@GuardedBy("requestLock")
private Priority priority;
private final Priority priority;

@GuardedBy("requestLock")
private Target<R> target;
private final Target<R> target;

@GuardedBy("requestLock")
@Nullable
private List<RequestListener<R>> requestListeners;
@Nullable private final List<RequestListener<R>> requestListeners;

@GuardedBy("requestLock")
private TransitionFactory<? super R> animationFactory;
private final TransitionFactory<? super R> animationFactory;

@GuardedBy("requestLock")
private Executor callbackExecutor;
private final Executor callbackExecutor;

@GuardedBy("requestLock")
private Resource<R> resource;
Expand All @@ -141,12 +108,15 @@ private enum Status {
private Status status;

@GuardedBy("requestLock")
@Nullable
private Drawable errorDrawable;

@GuardedBy("requestLock")
@Nullable
private Drawable placeholderDrawable;

@GuardedBy("requestLock")
@Nullable
private Drawable fallbackDrawable;

@GuardedBy("requestLock")
Expand All @@ -163,7 +133,7 @@ private enum Status {
public static <R> SingleRequest<R> obtain(
Context context,
GlideContext glideContext,
@Nullable Object requestLock,
Object requestLock,
Object model,
Class<R> transcodeClass,
BaseRequestOptions<?> requestOptions,
Expand All @@ -177,15 +147,7 @@ public static <R> SingleRequest<R> obtain(
Engine engine,
TransitionFactory<? super R> animationFactory,
Executor callbackExecutor) {
@SuppressWarnings("unchecked")
SingleRequest<R> request = (SingleRequest<R>) POOL.acquire();
if (request == null) {
request = new SingleRequest<>();
}
if (requestLock == null) {
requestLock = request;
}
request.init(
return new SingleRequest<>(
context,
glideContext,
requestLock,
Expand All @@ -202,93 +164,50 @@ public static <R> SingleRequest<R> obtain(
engine,
animationFactory,
callbackExecutor);
return request;
}

@SuppressWarnings("WeakerAccess")
@Synthetic
SingleRequest() {
// just create, instances are reused with recycle/init
}

// We are in fact locking on the same lock that will be used for all subsequent method calls.
@SuppressWarnings("GuardedBy")
private void init(
private SingleRequest(
Context context,
GlideContext glideContext,
@NonNull Object requestLock,
Object model,
@Nullable Object model,
Class<R> transcodeClass,
BaseRequestOptions<?> requestOptions,
int overrideWidth,
int overrideHeight,
Priority priority,
Target<R> target,
RequestListener<R> targetListener,
@Nullable RequestListener<R> targetListener,
@Nullable List<RequestListener<R>> requestListeners,
RequestCoordinator requestCoordinator,
Engine engine,
TransitionFactory<? super R> animationFactory,
Executor callbackExecutor) {
this.requestLock = Preconditions.checkNotNull(requestLock);
synchronized (this.requestLock) {
this.context = context;
this.glideContext = glideContext;
this.model = model;
this.transcodeClass = transcodeClass;
this.requestOptions = requestOptions;
this.overrideWidth = overrideWidth;
this.overrideHeight = overrideHeight;
this.priority = priority;
this.target = target;
this.targetListener = targetListener;
this.requestListeners = requestListeners;
this.requestCoordinator = requestCoordinator;
this.engine = engine;
this.animationFactory = animationFactory;
this.callbackExecutor = callbackExecutor;
status = Status.PENDING;

if (requestOrigin == null && glideContext.isLoggingRequestOriginsEnabled()) {
requestOrigin = new RuntimeException("Glide request origin trace");
}
this.requestLock = requestLock;
this.context = context;
this.glideContext = glideContext;
this.model = model;
this.transcodeClass = transcodeClass;
this.requestOptions = requestOptions;
this.overrideWidth = overrideWidth;
this.overrideHeight = overrideHeight;
this.priority = priority;
this.target = target;
this.targetListener = targetListener;
this.requestListeners = requestListeners;
this.requestCoordinator = requestCoordinator;
this.engine = engine;
this.animationFactory = animationFactory;
this.callbackExecutor = callbackExecutor;
status = Status.PENDING;

if (requestOrigin == null && glideContext.isLoggingRequestOriginsEnabled()) {
requestOrigin = new RuntimeException("Glide request origin trace");
}
}

@NonNull
@Override
public StateVerifier getVerifier() {
return stateVerifier;
}

@Override
public void recycle() {
synchronized (requestLock) {
assertNotCallingCallbacks();
context = null;
glideContext = null;
model = null;
transcodeClass = null;
requestOptions = null;
overrideWidth = -1;
overrideHeight = -1;
target = null;
requestListeners = null;
targetListener = null;
requestCoordinator = null;
animationFactory = null;
loadStatus = null;
errorDrawable = null;
placeholderDrawable = null;
fallbackDrawable = null;
width = -1;
height = -1;
requestOrigin = null;
POOL.release(this);
}
requestLock = null;
}

@Override
public void begin() {
synchronized (requestLock) {
Expand Down Expand Up @@ -438,6 +357,11 @@ public boolean isCleared() {
}
}

@Override
public void recycle() {
// TODO: remove this method, it's a no-op.
}

@GuardedBy("requestLock")
private Drawable getErrorDrawable() {
if (errorDrawable == null) {
Expand Down

0 comments on commit bee6348

Please sign in to comment.