From cacd8e14ca7eaeb66c43f544eed37f2facdb5aa0 Mon Sep 17 00:00:00 2001 From: Ben Manes Date: Mon, 23 Mar 2015 15:19:03 -0700 Subject: [PATCH] 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. --- .../caffeine/cache/NodeGenerator.java | 21 ++++--- .../caffeine/cache/AsyncLoadingCache.java | 4 +- .../caffeine/cache/BoundedLocalCache.java | 57 ++++++++++++------- .../github/benmanes/caffeine/cache/Node.java | 16 ------ .../caffeine/cache/BoundedLocalCacheTest.java | 25 ++++---- 5 files changed, 61 insertions(+), 62 deletions(-) diff --git a/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/NodeGenerator.java b/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/NodeGenerator.java index e4d167de5a..75b7255cea 100644 --- a/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/NodeGenerator.java +++ b/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(); } diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncLoadingCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncLoadingCache.java index e1a891b052..68844bbcba 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncLoadingCache.java +++ b/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. *

* Implementations of this interface are expected to be thread-safe, and can be safely accessed * by multiple concurrent threads. diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java index d4324ce703..a27f15812f 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java @@ -470,7 +470,7 @@ void afterRead(Node 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 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 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 orderedMap(LinkedDeque> deque, Function transformer, Node 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 sortedByWriteTime(boolean ascending, int limit) { Node 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); } } diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Node.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Node.java index 638edc4bb1..b65218a3f2 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Node.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Node.java @@ -73,22 +73,6 @@ interface Node extends AccessOrder>, WriteOrder> { /** If the entry was removed from the hash-table and the page replacement policy. */ boolean isDead(); - /** - * Atomically transitions the node from the alive state to the retired 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 retired state. */ @GuardedBy("this") void retire(); diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java index 5dd7b7905b..da874bae2f 100644 --- a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java +++ b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java @@ -108,16 +108,21 @@ public void evict_alreadyRemoved(Cache cache, CacheContext con try { Object keyRef = localCache.nodeFactory.newLookupKey(oldEntry.getKey()); Node 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 cache, CacheContext con enum Status { ALIVE, RETIRED, DEAD } - static void checkStatus(BoundedLocalCache localCache, - Node 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 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)); } }