Skip to content

Commit

Permalink
Avoid starting identical requests.
Browse files Browse the repository at this point in the history
If a request is already in progress we should only
cancel and restart it if the new request differs
from the in progress request in some way. Prior
to this change, we always cleared and restarted 
requests without checking to see if the request
parameters had changed. 

Progress toward #2194.
  • Loading branch information
sjudd committed Aug 4, 2017
1 parent 7004e59 commit 9d10097
Show file tree
Hide file tree
Showing 9 changed files with 313 additions and 9 deletions.
10 changes: 7 additions & 3 deletions library/src/main/java/com/bumptech/glide/RequestBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -353,14 +353,18 @@ public <Y extends Target<TranscodeType>> Y into(@NonNull Y target) {
throw new IllegalArgumentException("You must call #load() before calling #into()");
}

Request previous = target.getRequest();
requestOptions.lock();
Request request = buildRequest(target);

Request previous = target.getRequest();
if (previous != null) {
if (request.isEquivalentTo(previous)) {
request.recycle();
return target;
}
requestManager.clear(target);
}

requestOptions.lock();
Request request = buildRequest(target);
target.setRequest(request);
requestManager.track(target, request);

Expand Down
14 changes: 14 additions & 0 deletions library/src/main/java/com/bumptech/glide/request/Request.java
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,18 @@ public interface Request {
* Recycles the request object and releases its resources.
*/
void recycle();

/**
* Returns {@code true} if this {@link Request} is equivalent to the given {@link Request} (has
* all of the same options and sizes).
*
* <p>This method is identical to {@link #equals(Object)} except that it's specific to
* {@link Request} subclasses. We do not use {@link #equals(Object)} directly because we track
* {@link Request}s in collections like {@link java.util.Set} and it's perfectly legitimate to
* have two different {@link Request} objects for two different
* {@link com.bumptech.glide.request.target.Target}s (for example). Using a similar but different
* method let's us selectively compare {@link Request} objects to each other when it's useful in
* specific scenarios.
*/
boolean isEquivalentTo(Request other);
}
Original file line number Diff line number Diff line change
Expand Up @@ -1095,6 +1095,62 @@ public RequestOptions apply(RequestOptions other) {
return selfOrThrowIfLocked();
}


@Override
public boolean equals(Object o) {
if (o instanceof RequestOptions) {
RequestOptions other = (RequestOptions) o;
return Float.compare(other.sizeMultiplier, sizeMultiplier) == 0
&& errorId == other.errorId
&& Util.bothNullOrEqual(errorPlaceholder, other.errorPlaceholder)
&& placeholderId == other.placeholderId
&& Util.bothNullOrEqual(placeholderDrawable, other.placeholderDrawable)
&& fallbackId == other.fallbackId
&& Util.bothNullOrEqual(fallbackDrawable, other.fallbackDrawable)
&& isCacheable == other.isCacheable
&& overrideHeight == other.overrideHeight
&& overrideWidth == other.overrideWidth
&& isTransformationRequired == other.isTransformationRequired
&& isTransformationAllowed == other.isTransformationAllowed
&& useUnlimitedSourceGeneratorsPool == other.useUnlimitedSourceGeneratorsPool
&& onlyRetrieveFromCache == other.onlyRetrieveFromCache
&& diskCacheStrategy.equals(other.diskCacheStrategy)
&& priority == other.priority
&& options.equals(other.options)
&& transformations.equals(other.transformations)
&& resourceClass.equals(other.resourceClass)
&& Util.bothNullOrEqual(signature, other.signature)
&& Util.bothNullOrEqual(theme, other.theme);
}
return false;
}

@Override
public int hashCode() {
int hashCode = Util.hashCode(sizeMultiplier);
hashCode = Util.hashCode(errorId, hashCode);
hashCode = Util.hashCode(errorPlaceholder, hashCode);
hashCode = Util.hashCode(placeholderId, hashCode);
hashCode = Util.hashCode(placeholderDrawable, hashCode);
hashCode = Util.hashCode(fallbackId, hashCode);
hashCode = Util.hashCode(fallbackDrawable, hashCode);
hashCode = Util.hashCode(isCacheable, hashCode);
hashCode = Util.hashCode(overrideHeight, hashCode);
hashCode = Util.hashCode(overrideWidth, hashCode);
hashCode = Util.hashCode(isTransformationRequired, hashCode);
hashCode = Util.hashCode(isTransformationAllowed, hashCode);
hashCode = Util.hashCode(useUnlimitedSourceGeneratorsPool, hashCode);
hashCode = Util.hashCode(onlyRetrieveFromCache, hashCode);
hashCode = Util.hashCode(diskCacheStrategy, hashCode);
hashCode = Util.hashCode(priority, hashCode);
hashCode = Util.hashCode(options, hashCode);
hashCode = Util.hashCode(transformations, hashCode);
hashCode = Util.hashCode(resourceClass, hashCode);
hashCode = Util.hashCode(signature, hashCode);
hashCode = Util.hashCode(theme, hashCode);
return hashCode;
}

/**
* Throws if any further mutations are attempted.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ private enum Status {
PAUSED,
}

private final String tag = String.valueOf(hashCode());
private final String tag = String.valueOf(super.hashCode());
private final StateVerifier stateVerifier = StateVerifier.newInstance();

private RequestCoordinator requestCoordinator;
Expand Down Expand Up @@ -551,6 +551,20 @@ private void onLoadFailed(GlideException e, int maxLogLevel) {
}
}

@Override
public boolean isEquivalentTo(Request o) {
if (o instanceof SingleRequest) {
SingleRequest that = (SingleRequest) o;
return overrideWidth == that.overrideWidth
&& overrideHeight == that.overrideHeight
&& model.equals(that.model)
&& transcodeClass.equals(that.transcodeClass)
&& requestOptions.equals(that.requestOptions)
&& priority == that.priority;
}
return false;
}

private void logV(String message) {
Log.v(TAG, message + " this: " + tag);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,14 @@ public void recycle() {
full.recycle();
thumb.recycle();
}

@Override
public boolean isEquivalentTo(Request o) {
if (o instanceof ThumbnailRequestCoordinator) {
ThumbnailRequestCoordinator that = (ThumbnailRequestCoordinator) o;
return (full == null ? that.full == null : full.isEquivalentTo(that.full))
&& (thumb == null ? that.thumb == null : thumb.isEquivalentTo(that.thumb));
}
return false;
}
}
31 changes: 31 additions & 0 deletions library/src/main/java/com/bumptech/glide/util/Util.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
* A collection of assorted utility classes.
*/
public final class Util {
private static final int HASH_MULTIPLIER = 31;
private static final int HASH_ACCUMULATOR = 17;
private static final char[] HEX_CHAR_ARRAY = "0123456789abcdef".toCharArray();
// 32 bytes from sha-256 -> 64 hex chars.
private static final char[] SHA_256_CHARS = new char[64];
Expand Down Expand Up @@ -187,4 +189,33 @@ public static <T> List<T> getSnapshot(Collection<T> other) {
public static boolean bothNullOrEqual(Object a, Object b) {
return a == null ? b == null : a.equals(b);
}

public static int hashCode(int value) {
return hashCode(value, HASH_ACCUMULATOR);
}

public static int hashCode(int value, int accumulator) {
return accumulator * HASH_MULTIPLIER + value;
}

public static int hashCode(float value) {
return hashCode(value, HASH_ACCUMULATOR);
}

public static int hashCode(float value, int accumulator) {
return hashCode(Float.floatToIntBits(value), accumulator);
}

public static int hashCode(Object object, int accumulator) {
return hashCode(object == null ? 0 : object.hashCode(), accumulator);
}

public static int hashCode(boolean value, int accumulator) {
return hashCode(value ? 1 : 0, accumulator);
}

public static int hashCode(boolean value) {
return hashCode(value, HASH_ACCUMULATOR);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,29 @@

import static com.google.common.truth.Truth.assertThat;

import android.app.Application;
import android.graphics.Bitmap;

import android.graphics.Color;
import android.graphics.drawable.ColorDrawable;
import android.graphics.drawable.Drawable;
import android.graphics.drawable.GradientDrawable;
import com.bumptech.glide.Priority;
import com.bumptech.glide.load.MultiTransformation;
import com.bumptech.glide.load.Option;
import com.bumptech.glide.load.Transformation;
import com.bumptech.glide.load.engine.DiskCacheStrategy;
import com.bumptech.glide.load.resource.bitmap.CenterCrop;
import com.bumptech.glide.load.resource.bitmap.CircleCrop;

import com.bumptech.glide.signature.ObjectKey;
import com.bumptech.glide.util.Util;
import com.google.common.testing.EqualsTester;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.robolectric.RobolectricTestRunner;
import org.robolectric.RuntimeEnvironment;
import org.robolectric.annotation.Config;

@RunWith(RobolectricTestRunner.class)
Expand All @@ -23,11 +33,14 @@ public class RequestOptionsTest {

private RequestOptions options;
@Mock private Transformation<Bitmap> transformation;
private Application app;

@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
options = new RequestOptions();

app = RuntimeEnvironment.application;
}

@Test
Expand Down Expand Up @@ -146,4 +159,101 @@ public void testApplyMultiTransform() {
assertThat(options.getTransformations().get(Bitmap.class))
.isInstanceOf(MultiTransformation.class);
}

@Test
public void testEqualsHashCode() {
Drawable first = new ColorDrawable(Color.RED);
Drawable second = new GradientDrawable();
assertThat(first).isNotEqualTo(second);
assertThat(Util.bothNullOrEqual(first, second)).isFalse();
new EqualsTester()
.addEqualityGroup(
new RequestOptions().sizeMultiplier(.7f),
new RequestOptions().sizeMultiplier(.7f))
.addEqualityGroup(new RequestOptions().sizeMultiplier(0.8f))
.addEqualityGroup(new RequestOptions().error(1), new RequestOptions().error(1))
.addEqualityGroup(new RequestOptions().error(2))
.addEqualityGroup(new RequestOptions().error(first), new RequestOptions().error(first))
.addEqualityGroup(new RequestOptions().error(second))
.addEqualityGroup(new RequestOptions().placeholder(1), new RequestOptions().placeholder(1))
.addEqualityGroup(new RequestOptions().placeholder(2))
.addEqualityGroup(
new RequestOptions().placeholder(first),
new RequestOptions().placeholder(first))
.addEqualityGroup(new RequestOptions().placeholder(second))
.addEqualityGroup(new RequestOptions().fallback(1), new RequestOptions().fallback(1))
.addEqualityGroup(new RequestOptions().fallback(2))
.addEqualityGroup(
new RequestOptions().fallback(first),
new RequestOptions().fallback(first))
.addEqualityGroup(new RequestOptions().fallback(second))
.addEqualityGroup(
new RequestOptions().skipMemoryCache(true),
new RequestOptions().skipMemoryCache(true))
.addEqualityGroup(
new RequestOptions(),
new RequestOptions().skipMemoryCache(false),
new RequestOptions().theme(null),
new RequestOptions().onlyRetrieveFromCache(false),
new RequestOptions().useUnlimitedSourceGeneratorsPool(false))
.addEqualityGroup(
new RequestOptions().override(100),
new RequestOptions().override(100, 100))
.addEqualityGroup(
new RequestOptions().override(200),
new RequestOptions().override(200, 200))
.addEqualityGroup(
new RequestOptions().override(100, 200),
new RequestOptions().override(100, 200))
.addEqualityGroup(
new RequestOptions().override(200, 100),
new RequestOptions().override(200, 100))
.addEqualityGroup(
new RequestOptions().centerCrop(),
new RequestOptions().centerCrop())
.addEqualityGroup(
new RequestOptions().optionalCenterCrop(),
new RequestOptions().optionalCenterCrop())
.addEqualityGroup(new RequestOptions().fitCenter())
.addEqualityGroup(new RequestOptions().circleCrop())
.addEqualityGroup(new RequestOptions().centerInside())
.addEqualityGroup(
new RequestOptions().useUnlimitedSourceGeneratorsPool(true),
new RequestOptions().useUnlimitedSourceGeneratorsPool(true))
.addEqualityGroup(
new RequestOptions().onlyRetrieveFromCache(true),
new RequestOptions().onlyRetrieveFromCache(true))
.addEqualityGroup(
new RequestOptions().diskCacheStrategy(DiskCacheStrategy.ALL),
new RequestOptions().diskCacheStrategy(DiskCacheStrategy.ALL))
.addEqualityGroup(
new RequestOptions().diskCacheStrategy(DiskCacheStrategy.NONE))
.addEqualityGroup(
new RequestOptions().priority(Priority.HIGH),
new RequestOptions().priority(Priority.HIGH))
.addEqualityGroup(
new RequestOptions().priority(Priority.LOW))
.addEqualityGroup(
new RequestOptions().set(Option.<Boolean>memory("test"), true),
new RequestOptions().set(Option.<Boolean>memory("test"), true))
.addEqualityGroup(
new RequestOptions().set(Option.<Boolean>memory("test"), false))
.addEqualityGroup(
new RequestOptions().set(Option.<Boolean>memory("test2"), true))
.addEqualityGroup(
new RequestOptions().decode(Integer.class),
new RequestOptions().decode(Integer.class))
.addEqualityGroup(
new RequestOptions().decode(Float.class))
.addEqualityGroup(
new RequestOptions().signature(new ObjectKey("test")),
new RequestOptions().signature(new ObjectKey("test")))
.addEqualityGroup(
new RequestOptions().signature(new ObjectKey("test2")))
.addEqualityGroup(
new RequestOptions().theme(app.getTheme()),
new RequestOptions().theme(app.getTheme()))
.testEquals();
}

}
Loading

0 comments on commit 9d10097

Please sign in to comment.