Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Iterator.remove when trimming cache #2550

Merged
merged 3 commits into from
Nov 7, 2017
Merged

Use Iterator.remove when trimming cache #2550

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f86c3fe
Use Iterator.remove when trimming cache so even keys violating equals…
unverbraucht Nov 2, 2017
e202acf
Ensure that we don't overrun the iterator
unverbraucht Nov 2, 2017
f5132b2
Get new iterator for each iteration so concurrent modification is pos…
unverbraucht Nov 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions library/src/main/java/com/bumptech/glide/util/LruCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package com.bumptech.glide.util;

import android.support.annotation.Nullable;

import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.Map;

Expand Down Expand Up @@ -166,12 +168,14 @@ public void clearMemory() {
*/
protected synchronized void trimToSize(int size) {
Map.Entry<T, Y> last;
Iterator<Map.Entry<T, Y>> cacheIterator;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a preference (declare close to usage, and in scope): these two vars are only used within the loop, so they could be defined inside:

final Iterator<Map.Entry<T, Y>> cacheIterator  = cache.entrySet().iterator();
final Map.Entry<T, Y> last = cacheIterator.next();

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree in principle, but this matches the existing style, so let's just leave it as is for now.

while (currentSize > size) {
last = cache.entrySet().iterator().next();
cacheIterator = cache.entrySet().iterator();
last = cacheIterator.next();
final Y toRemove = last.getValue();
currentSize -= getSize(toRemove);
final T key = last.getKey();
cache.remove(key);
cacheIterator.remove();
onItemEvicted(key, toRemove);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a chance of ConcurrentModificationException because of this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, using the Iterator to remove is safe when using the Iterator itself to loop over the collection. I added another commit to ensure that we don't overrun the collection, although I don't see how it could happen unless the size to trim to is <0. Can't hurt though I guess.

Copy link
Collaborator

@TWiStErRob TWiStErRob Nov 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sjudd before you merge, please consider this test (it throws with new code):

  @Test
  public void testPreventEviction() {
    final MemoryCache cache = new LruResourceCache(100);
    final Resource<?> first = getResource(30);
    final Key firstKey = new MockKey();
    cache.put(firstKey, first);
    Resource<?> second = getResource(30);
    Key secondKey = new MockKey();
    cache.put(secondKey, second);
    Resource<?> third = getResource(30);
    Key thirdKey = new MockKey();
    cache.put(thirdKey, third);
    cache.setResourceRemovedListener(new ResourceRemovedListener() {
      @Override
      public void onResourceRemoved(Resource<?> removed) {
        if (removed == first) {
          cache.put(firstKey, first);
        }
      }
    });

    // trims from 100 to 50, having 30+30+30 items, it should trim to 1 item
    cache.trimMemory(ComponentCallbacks2.TRIM_MEMORY_UI_HIDDEN);

    // and that 1 item must be first, because it's forced to return to cache in the listener
    @SuppressWarnings("unchecked")
    LruCache<Key, Resource<?>> lruCache = (LruCache<Key, Resource<?>>) cache;
    assertTrue(lruCache.contains(firstKey));
    assertFalse(lruCache.contains(secondKey));
    assertFalse(lruCache.contains(thirdKey));
  }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unverbraucht thoughts? I don't think we typically have concurrent modifications like this, but it's probably possible (and may be why the code was written the way it was initially).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sjudd @TWiStErRob good point. It might make sense then to get a fresh instance of the Iterator after every loop run. I've committed proposed changes for that.

}
}
Expand Down