AsyncCache#getAll could skip storing additional mappings (fixes #655)

ben-manes · Jan 25, 2022 · bea9b2d · bea9b2d
1 parent d212cd6
commit bea9b2d
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 17 deletions.
diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java
@@ -275,9 +275,6 @@ private void fillProxies(Map<? extends K, ? extends V> result) {
 
     /** Adds to the cache any extra entries computed that were not requested. */
     private void addNewEntries(Map<? extends K, ? extends V> result) {
-      if (proxies.size() == result.size()) {
-        return;
-      }
       result.forEach((key, value) -> {
         if (!proxies.containsKey(key)) {
           cache.put(key, CompletableFuture.completedFuture(value));

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncCacheTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncCacheTest.java
@@ -25,6 +25,7 @@
 import static com.google.common.truth.Truth.assertThat;
 import static java.util.function.Function.identity;
 import static java.util.stream.Collectors.toMap;
+import static java.util.stream.Collectors.toSet;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
@@ -436,6 +437,25 @@ public void getAllFunction_exceeds(AsyncCache<Int, Int> cache, CacheContext cont
     assertThat(context).stats().hits(0).misses(result.size()).success(1).failures(0);
   }
 
+  @Test(dataProvider = "caches")
+  @CacheSpec(removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void getAllFunction_different(AsyncCache<Int, Int> cache, CacheContext context) {
+    int requested = ThreadLocalRandom.current().nextInt(1, context.absent().size() / 2);
+    var requestedKeys = context.absentKeys().stream().limit(requested).collect(toSet());
+    var actual = context.absent().entrySet().stream()
+        .filter(entry -> !requestedKeys.contains(entry.getKey()))
+        .limit(requested)
+        .collect(toMap(Map.Entry::getKey, Map.Entry::getValue));
+    var result = cache.getAll(requestedKeys, keys -> {
+      return actual;
+    }).join();
+
+    assertThat(result).isEmpty();
+    assertThat(cache).hasSize(context.initialSize() + actual.size());
+    assertThat(cache.synchronous().asMap()).containsAtLeastEntriesIn(actual);
+    assertThat(context).stats().hits(0).misses(actual.size()).success(1).failures(0);
+  }
+
   @Test(dataProvider = "caches")
   @CacheSpec(population = { Population.SINGLETON, Population.PARTIAL, Population.FULL },
       removalListener = { Listener.DEFAULT, Listener.REJECTING })
@@ -653,6 +673,25 @@ public void getAllBifunction_exceeds(AsyncCache<Int, Int> cache, CacheContext co
     assertThat(context).stats().hits(0).misses(result.size()).success(1).failures(0);
   }
 
+  @Test(dataProvider = "caches")
+  @CacheSpec(removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void getAllBifunction_different(AsyncCache<Int, Int> cache, CacheContext context) {
+    int requested = ThreadLocalRandom.current().nextInt(1, context.absent().size() / 2);
+    var requestedKeys = context.absentKeys().stream().limit(requested).collect(toSet());
+    var actual = context.absent().entrySet().stream()
+        .filter(entry -> !requestedKeys.contains(entry.getKey()))
+        .limit(requested)
+        .collect(toMap(Map.Entry::getKey, Map.Entry::getValue));
+    var result = cache.getAll(requestedKeys, (keys, executor) -> {
+      return CompletableFuture.completedFuture(actual);
+    }).join();
+
+    assertThat(result).isEmpty();
+    assertThat(cache).hasSize(context.initialSize() + actual.size());
+    assertThat(cache.synchronous().asMap()).containsAtLeastEntriesIn(actual);
+    assertThat(context).stats().hits(0).misses(actual.size()).success(1).failures(0);
+  }
+
   @Test(dataProvider = "caches")
   @CacheSpec(population = { Population.SINGLETON, Population.PARTIAL, Population.FULL },
       removalListener = { Listener.DEFAULT, Listener.REJECTING })

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncLoadingCacheTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncLoadingCacheTest.java
@@ -24,6 +24,7 @@
 import static com.github.benmanes.caffeine.testing.MapSubject.assertThat;
 import static java.util.function.Function.identity;
 import static java.util.stream.Collectors.toMap;
+import static java.util.stream.Collectors.toSet;
 
 import java.util.ArrayList;
 import java.util.Collections;
@@ -34,6 +35,7 @@
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
 import java.util.concurrent.Executor;
+import java.util.concurrent.ThreadLocalRandom;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.BiFunction;
@@ -56,9 +58,6 @@
 import com.github.benmanes.caffeine.cache.testing.CacheValidationListener;
 import com.github.benmanes.caffeine.cache.testing.CheckNoStats;
 import com.github.benmanes.caffeine.testing.Int;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Iterables;
-import com.google.common.primitives.Ints;
 
 /**
  * The test cases for the {@link AsyncLoadingCache} interface that simulate the most generic usages.
@@ -218,20 +217,33 @@ public void getAll_exceeds(AsyncLoadingCache<Int, Int> cache, CacheContext conte
   }
 
   @Test(dataProvider = "caches")
-  @CacheSpec(loader = { Loader.NEGATIVE, Loader.BULK_NEGATIVE },
-      population = { Population.SINGLETON, Population.PARTIAL, Population.FULL },
+  @CacheSpec(removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void getAll_different(AsyncCache<Int, Int> cache, CacheContext context) {
+    int requested = ThreadLocalRandom.current().nextInt(1, context.absent().size() / 2);
+    var requestedKeys = context.absentKeys().stream().limit(requested).collect(toSet());
+    var actual = context.absent().entrySet().stream()
+        .filter(entry -> !requestedKeys.contains(entry.getKey()))
+        .limit(requested)
+        .collect(toMap(Map.Entry::getKey, Map.Entry::getValue));
+    var result = cache.getAll(requestedKeys, (keys, executor) -> {
+      return CompletableFuture.completedFuture(actual);
+    }).join();
+
+    assertThat(result).isEmpty();
+    assertThat(cache).hasSize(context.initialSize() + actual.size());
+    assertThat(cache.synchronous().asMap()).containsAtLeastEntriesIn(actual);
+    assertThat(context).stats().hits(0).misses(actual.size()).success(1).failures(0);
+  }
+
+  @Test(dataProvider = "caches")
+  @CacheSpec(loader = Loader.BULK_DIFFERENT,
       removalListener = { Listener.DEFAULT, Listener.REJECTING })
   public void getAll_duplicates(AsyncLoadingCache<Int, Int> cache, CacheContext context) {
-    var absentKeys = ImmutableSet.copyOf(Iterables.limit(context.absentKeys(),
-        Ints.saturatedCast(context.maximum().max() - context.initialSize())));
-    var keys = Iterables.concat(absentKeys, absentKeys,
-        context.original().keySet(), context.original().keySet());
-    var result = cache.getAll(keys).join();
-    assertThat(result).containsExactlyKeys(keys);
+    var result = cache.getAll(context.absentKeys()).join();
 
-    int loads = context.loader().isBulk() ? 1 : absentKeys.size();
-    assertThat(context).stats().hits(context.initialSize())
-        .misses(absentKeys.size()).success(loads).failures(0);
+    assertThat(result).isEmpty();
+    assertThat(cache.asMap()).containsAtLeastEntriesIn(result);
+    assertThat(context).stats().hits(0).misses(context.absent().size()).success(1).failures(0);
   }
 
   @Test(dataProvider = "caches")

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/CacheSpec.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/CacheSpec.java
@@ -476,6 +476,19 @@ enum Loader implements CacheLoader<Int, Int> {
         return result;
       }
     },
+    /** A bulk-only loader that loads only keys that were not requested. */
+    BULK_DIFFERENT {
+      @Override public Int load(Int key) {
+        throw new UnsupportedOperationException();
+      }
+      @Override public Map<Int, Int> loadAll(Set<? extends Int> keys) throws Exception {
+        var result = new HashMap<Int, Int>(keys.size());
+        for (Int key : keys) {
+          result.put(key.negate(), key);
+        }
+        return result;
+      }
+    },
     /** A bulk-only loader that loads more than requested. */
     BULK_NEGATIVE_EXCEEDS {
       @Override public Int load(Int key) {