Use relaxed reads for key

The key fixed unless the node transitions to the retired or dead state under
the node's lock. When it does, the entry should not be visible from the caller's
perspective. This was audited to verify correct usages.

The conditional remove was modified to follow the standard pattern of remove,
then transition. It was implemented as a conditional transition followed by a
remove. This was legacy from CLHM where compute() methods had not originally
been available. By using computeIfPresent to conditionally remove, we have more
assurance of the correct behavior. The previous may have even been wrong if the
value had been weak/soft GC'd.

Relaxed reads should offer a slight performance improvement due to avoiding
unnecessary memory barriers.

Author: ben-manes

caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/NodeGenerator.java

@@ -132,8 +132,8 @@ private void addKey() {
     nodeSubtype
         .addField(newFieldOffset(className, "key"))
         .addField(newKeyField())
-        .addMethod(newGetter(keyStrength, kTypeVar, "key", Visibility.IMMEDIATE))
-        .addMethod(newGetterRef("key"));
+        .addMethod(newGetter(keyStrength, kTypeVar, "key", Visibility.LAZY))
+        .addMethod(newGetKeyRef());
     addKeyConstructorAssignment(constructorByKey, false);
     addKeyConstructorAssignment(constructorByKeyRef, true);
   }
@@ -361,14 +361,14 @@ private void addStateMethods() {
         : baseClassName() + '.' + offsetName("key");
 
     nodeSubtype.addMethod(MethodSpec.methodBuilder("isAlive")
-        .addStatement("Object key = this.key")
+        .addStatement("Object key = getKeyReference()")
         .addStatement("return (key != $L) && (key != $L)", retiredArg, deadArg)
         .addModifiers(Modifier.PUBLIC, Modifier.FINAL)
         .returns(boolean.class)
         .build());
 
     nodeSubtype.addMethod(MethodSpec.methodBuilder("isRetired")
-        .addStatement("return (key == $L)", retiredArg)
+        .addStatement("return (getKeyReference() == $L)", retiredArg)
         .addModifiers(Modifier.PUBLIC, Modifier.FINAL)
         .returns(boolean.class)
         .build());
@@ -379,7 +379,7 @@ private void addStateMethods() {
         .build());
 
     nodeSubtype.addMethod(MethodSpec.methodBuilder("isDead")
-        .addStatement("return (key == $L)", deadArg)
+        .addStatement("return (getKeyReference() == $L)", deadArg)
         .addModifiers(Modifier.PUBLIC, Modifier.FINAL)
         .returns(boolean.class)
         .build());
@@ -397,14 +397,13 @@ private String baseClassName() {
     return Feature.makeClassName(keyAndValue);
   }
 
-  /** Creates an accessor that returns the reference holding the variable. */
-  private MethodSpec newGetterRef(String varName) {
-    String methodName = String.format("get%sReference",
-        Character.toUpperCase(varName.charAt(0)) + varName.substring(1));
-    MethodSpec.Builder getter = MethodSpec.methodBuilder(methodName)
+  /** Creates an accessor that returns the key reference. */
+  private MethodSpec newGetKeyRef() {
+    MethodSpec.Builder getter = MethodSpec.methodBuilder("getKeyReference")
         .addModifiers(Modifier.PUBLIC, Modifier.FINAL)
         .returns(Object.class);
-    getter.addStatement("return $N", varName);
+    getter.addStatement("return $T.UNSAFE.getObject(this, $N)",
+        UNSAFE_ACCESS, offsetName("key"));
     return getter.build();
   }
 

caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncLoadingCache.java

@@ -26,8 +26,8 @@
 import javax.annotation.concurrent.ThreadSafe;
 
 /**
- * A semi-persistent mapping from keys to values. Values are automatically loaded by the cache,
- * and are stored in the cache until either evicted or manually invalidated.
+ * A semi-persistent mapping from keys to values. Values are automatically loaded by the cache
+ * asynchronously, and are stored in the cache until either evicted or manually invalidated.
  * <p>
  * Implementations of this interface are expected to be thread-safe, and can be safely accessed
  * by multiple concurrent threads.

caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java

@@ -470,7 +470,7 @@ void afterRead(Node<K, V> node, boolean recordHit) {
       if (((now - writeTime) > refreshAfterWriteNanos()) && node.casWriteTime(writeTime, now)) {
         executor().execute(() -> {
           K key = node.getKey();
-          if (key != null) {
+          if ((key != null) && node.isAlive()) {
             try {
               computeIfPresent(key, cacheLoader()::reload);
             } catch (Throwable t) {
@@ -943,13 +943,23 @@ public V remove(Object key) {
       return null;
     }
 
-    V oldValue = node.makeRetired();
-    if (oldValue != null) {
+    boolean retired = false;
+    synchronized (node) {
+      if (node.isAlive()) {
+        retired = true;
+        node.retire();
+      }
+    }
+    V oldValue = node.getValue();
+    if (retired) {
       afterWrite(node, new RemovalTask(node));
       if (hasRemovalListener()) {
         @SuppressWarnings("unchecked")
         K castKey = (K) key;
-        notifyRemoval(castKey, oldValue, RemovalCause.EXPLICIT);
+        RemovalCause cause = (oldValue == null)
+            ? RemovalCause.COLLECTED
+            : RemovalCause.EXPLICIT;
+        notifyRemoval(castKey, oldValue, cause);
       }
     }
     return oldValue;
@@ -958,26 +968,31 @@ public V remove(Object key) {
   @Override
   public boolean remove(Object key, Object value) {
     Object keyRef = nodeFactory.newLookupKey(key);
-    final Node<K, V> node = data.get(keyRef);
     tracer().recordDelete(id, key);
-    if ((node == null) || (value == null)) {
+    if ((data.get(keyRef) == null) || (value == null)) {
       return false;
     }
-    V oldValue;
-    synchronized (node) {
-      oldValue = node.getValue();
-      if (node.isAlive() && node.containsValue(value)) {
-        node.retire();
-      } else {
-        return false;
+    @SuppressWarnings({"unchecked", "rawtypes"})
+    Node<K, V> removed[] = new Node[1];
+    data.computeIfPresent(keyRef, (k, node) -> {
+      synchronized (node) {
+        if (node.isAlive() && node.containsValue(value)) {
+          node.retire();
+        } else {
+          return node;
+        }
       }
-    }
-    if (data.remove(keyRef, node) && hasRemovalListener()) {
+      removed[0] = node;
+      return null;
+    });
+    if (removed[0] == null) {
+      return false;
+    } else if (hasRemovalListener()) {
       @SuppressWarnings("unchecked")
       K castKey = (K) key;
-      notifyRemoval(castKey, oldValue, RemovalCause.EXPLICIT);
+      notifyRemoval(castKey, removed[0].getValue(), RemovalCause.EXPLICIT);
     }
-    afterWrite(node, new RemovalTask(node));
+    afterWrite(removed[0], new RemovalTask(removed[0]));
     return true;
   }
 
@@ -1128,7 +1143,7 @@ public V computeIfPresent(K key,
         V oldValue = prior.getValue();
         newValue[0] = statsAware(remappingFunction, false, false).apply(key, oldValue);
         if (newValue[0] == null) {
-          prior.makeRetired();
+          prior.retire();
           task[0] = new RemovalTask(prior);
           if (hasRemovalListener()) {
             notifyRemoval(key, oldValue, RemovalCause.EXPLICIT);
@@ -1312,7 +1327,7 @@ Map<K, V> orderedMap(LinkedDeque<Node<K, V>> deque, Function<V, V> transformer,
         Node<K, V> node = iterator.next();
         K key = node.getKey();
         V value = transformer.apply(node.getValue());
-        if ((key != null) && (value != null)) {
+        if ((key != null) && (value != null) && node.isAlive()) {
           map.put(key, value);
         }
       }
@@ -1537,7 +1552,7 @@ public boolean hasNext() {
           next = iterator.next();
           value = next.getValue();
           key = next.getKey();
-          if (hasExpired(next, now) || (key == null) || (value == null)) {
+          if (hasExpired(next, now) || (key == null) || (value == null) || !next.isAlive()) {
             value = null;
             next = null;
             key = null;
@@ -1808,7 +1823,7 @@ private Map<K, V> sortedByWriteTime(boolean ascending, int limit) {
           Node<K, V> node = iterator.next();
           K key = node.getKey();
           V value = transformer.apply(node.getValue());
-          if ((key != null) && (value != null)) {
+          if ((key != null) && (value != null) && node.isAlive()) {
             map.put(key, value);
           }
         }

caffeine/src/main/java/com/github/benmanes/caffeine/cache/Node.java

@@ -73,22 +73,6 @@ interface Node<K, V> extends AccessOrder<Node<K, V>>, WriteOrder<Node<K, V>> {
   /** If the entry was removed from the hash-table and the page replacement policy. */
   boolean isDead();
 
-  /**
-   * Atomically transitions the node from the <tt>alive</tt> state to the <tt>retired</tt> state, if
-   * a valid transition.
-   *
-   * @return the retired weighted value if the transition was successful or null otherwise
-   */
-  default @Nullable V makeRetired() {
-    synchronized (this) {
-      if (!isAlive()) {
-        return null;
-      }
-      retire();
-      return getValue();
-    }
-  }
-
   /** Sets the node to the <tt>retired</tt> state. */
   @GuardedBy("this")
   void retire();

caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java

@@ -108,16 +108,21 @@ public void evict_alreadyRemoved(Cache<Integer, Integer> cache, CacheContext con
     try {
       Object keyRef = localCache.nodeFactory.newLookupKey(oldEntry.getKey());
       Node<Integer, Integer> node = localCache.data.get(keyRef);
-      checkStatus(localCache, node, Status.ALIVE);
+      checkStatus(node, Status.ALIVE);
       ConcurrentTestHarness.execute(() -> {
         localCache.put(newEntry.getKey(), newEntry.getValue());
         assertThat(localCache.remove(oldEntry.getKey()), is(oldEntry.getValue()));
       });
       Awaits.await().until(() -> localCache.containsKey(oldEntry.getKey()), is(false));
-      checkStatus(localCache, node, Status.RETIRED);
+      Awaits.await().until(() -> {
+        synchronized (node) {
+          return !node.isAlive();
+        }
+      });
+      checkStatus(node, Status.RETIRED);
       localCache.drainBuffers();
 
-      checkStatus(localCache, node, Status.DEAD);
+      checkStatus(node, Status.DEAD);
       assertThat(localCache.containsKey(newEntry.getKey()), is(true));
       assertThat(cache, hasRemovalNotifications(context, 1, RemovalCause.EXPLICIT));
     } finally {
@@ -127,15 +132,11 @@ public void evict_alreadyRemoved(Cache<Integer, Integer> cache, CacheContext con
 
   enum Status { ALIVE, RETIRED, DEAD }
 
-  static void checkStatus(BoundedLocalCache<Integer, Integer> localCache,
-      Node<Integer, Integer> node, Status expected) {
-    assertThat(node.isAlive(), is(expected == Status.ALIVE));
-    assertThat(node.isRetired(), is(expected == Status.RETIRED));
-    assertThat(node.isDead(), is(expected == Status.DEAD));
-
-    if (node.isDead()) {
-      node.makeRetired();
-      assertThat(node.isRetired(), is(false));
+  static void checkStatus(Node<Integer, Integer> node, Status expected) {
+    synchronized (node) {
+      assertThat(node.isAlive(), is(expected == Status.ALIVE));
+      assertThat(node.isRetired(), is(expected == Status.RETIRED));
+      assertThat(node.isDead(), is(expected == Status.DEAD));
     }
   }