Skip to content

Commit

Permalink
Fixed race causing an incorrect removal cause (fixes #412)
Browse files Browse the repository at this point in the history
This bug manifests due to an incorrectly optimized `Map.remove(key)`
implementation when there is no CacheWriter specified. If there is,
an alternative implementation is used which does not suffer from this
mistake. This change now uses that version in all cases.

The explicit removal and an eviction of the same entry can occur
concurrently. If the removal wins the race, it is responsible for
retiring the entry. However even after that entry is discarded, an
further evictions may be necessary to meet the bounding criteria
(e.g. weighted entries). Therefore the eviction thread eagerly
discards the entry from the policy's data structures so that it can
continue its evaluation. While the entry is synchronized in both
cases, the eviction thread could lose the removal but retire the
entry first. This lead to the removal seeing a dead entry and
miscommunicating the cause to the listener.

There is a modest performance difference in a microbenchmark because
of a slightly coarser locking. However most of the differences between
libraries is whether they decide to be linearizable or perform an
optimistic check to no-op if the entry is absent. The optimistic check
avoids locking but also can lead to a removal to not block waiting for
an in-flight computation to complete. While a valid choice, some users
have expressed a desire for removal to be pessimistic and is rare
enough that we follow their wishes.
  • Loading branch information
ben-manes committed Apr 27, 2020
1 parent 614fe60 commit 8121c1d
Show file tree
Hide file tree
Showing 22 changed files with 295 additions and 76 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,14 @@
public interface BasicCache<K, V> {

/** Returns the value stored in the cache, or null if not present. */
@Nullable
V get(@NonNull K key);
@Nullable V get(@NonNull K key);

/** Stores the value into the cache, replacing an existing mapping if present. */
void put(@NonNull K key, @NonNull V value);

/** Removes the entry from the cache, if present. */
void remove(@NonNull K key);

/** Invalidates all entries from the cache. */
void clear();

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/*
* Copyright 2014 Ben Manes. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.github.benmanes.caffeine.cache;

import java.util.Random;

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Group;
import org.openjdk.jmh.annotations.GroupThreads;
import org.openjdk.jmh.annotations.Level;
import org.openjdk.jmh.annotations.Param;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.TearDown;

import site.ycsb.generator.NumberGenerator;
import site.ycsb.generator.ScrambledZipfianGenerator;

/**
* A benchmark that loosely evaluates the put and removal performance of the cache. The cache is
* written using a Zipf distribution of keys is used to incur lock contention on popular entries.
* <p>
* The performance of this benchmark is expected to be unrealistic due to removing entries. A cache
* that optimistically skips locking when the entry is absent receives a benefit, but sacrifices
* linearizability with a computation. In real-world usages the rate of explicit writes is
* relatively rare compared to reads. Thus, this benchmark is only for diagnosing performance
* concerns and should not be used to compare implementations.
* <p>
* <pre>{@code
* ./gradlew jmh -PincludePattern=PutRemoveBenchmark
* }</pre>
*
* @author [email protected] (Ben Manes)
*/
@State(Scope.Group)
public class PutRemoveBenchmark {
private static final int SIZE = (2 << 14);
private static final int MASK = SIZE - 1;
private static final int ITEMS = SIZE / 3;

@Param({
"Caffeine",
"LinkedHashMap_Lru",
"ConcurrentHashMap",
"ConcurrentLinkedHashMap",
"Guava",
"Cache2k",
"Ehcache3",
})
CacheType cacheType;

BasicCache<Integer, Boolean> cache;
Integer[] ints;

@State(Scope.Thread)
public static class ThreadState {
static final Random random = new Random();
int index = random.nextInt();
}

@Setup
public void setup() {
ints = new Integer[SIZE];
cache = cacheType.create(2 * SIZE);

// Enforce full initialization of internal structures
for (int i = 0; i < 2 * SIZE; i++) {
cache.put(i, Boolean.TRUE);
}
cache.clear();

// Populate with a realistic access distribution
NumberGenerator generator = new ScrambledZipfianGenerator(ITEMS);
for (int i = 0; i < SIZE; i++) {
ints[i] = generator.nextValue().intValue();
cache.put(ints[i], Boolean.TRUE);
}
}

@TearDown(Level.Iteration)
public void tearDown() {
cache.cleanUp();
}

@Benchmark @Group @GroupThreads(4)
public void put(ThreadState threadState) {
cache.put(ints[threadState.index++ & MASK], Boolean.TRUE);
}

@Benchmark @Group @GroupThreads(4)
public void remove(ThreadState threadState) {
cache.remove(ints[threadState.index++ & MASK]);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public final class Cache2k<K, V> implements BasicCache<K, V> {

@SuppressWarnings("unchecked")
public Cache2k(int maximumSize) {
cache = (Cache<K, V>) Cache2kBuilder.forUnknownTypes()
cache = Cache2kBuilder.forUnknownTypes()
.entryCapacity(maximumSize)
.eternal(true)
.build();
Expand All @@ -44,6 +44,11 @@ public void put(K key, V value) {
cache.put(key, value);
}

@Override
public void remove(K key) {
cache.remove(key);
}

@Override
public void clear() {
cache.clear();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ public void put(K key, V value) {
map.put(key, value);
}

@Override
public void remove(K key) {
map.remove(key);
}

@Override
public void clear() {
cache.invalidateAll();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ public void put(K key, V value) {
cache.putReplace(key, value);
}

@Override
public void remove(K key) {
cache.remove(key);
}

@Override
public void clear() {
cache.clear();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ public void put(K key, V value) {
map.put(key, value);
}

@Override
public void remove(K key) {
map.remove(key);
}

@Override
public void clear() {
map.clear();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ public void put(K key, V value) {
cache.put(key, value);
}

@Override
public void remove(K key) {
cache.remove(key);
}

@Override
public void clear() {
cache.clear();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ public void put(K key, V value) {
cache.put(key, value);
}

@Override
public void remove(K key) {
cache.invalidate(key);
}

@Override
public void clear() {
cache.invalidateAll();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ public void put(K key, V value) {
cache.put(key, value);
}

@Override
public void remove(K key) {
cache.remove(key);
}

@Override
public void clear() {
cache.clear();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ public void put(K key, V value) {
cache.put(key, value);
}

@Override
public void remove(K key) {
cache.invalidate(key);
}

@Override
public void clear() {
cache.invalidateAll();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@ public void put(K key, V value) {
}
}

@Override
public void remove(K key) {
synchronized (map) {
map.remove(key);
}
}

@Override
public void clear() {
synchronized (map) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ public void put(K key, V value) {
cache.put(key, value);
}

@Override
public void remove(K key) {
cache.remove(key);
}

@Override
public void clear() {
cache.clear();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2088,56 +2088,6 @@ public Map<K, V> getAllPresent(Iterable<?> keys) {

@Override
public @Nullable V remove(Object key) {
return hasWriter()
? removeWithWriter(key)
: removeNoWriter(key);
}

/**
* Removes the mapping for a key without notifying the writer.
*
* @param key key whose mapping is to be removed
* @return the removed value or null if no mapping was found
*/
@Nullable V removeNoWriter(Object key) {
Node<K, V> node = data.remove(nodeFactory.newLookupKey(key));
if (node == null) {
return null;
}

V oldValue;
synchronized (node) {
oldValue = node.getValue();
if (node.isAlive()) {
node.retire();
}
}

RemovalCause cause;
if (oldValue == null) {
cause = RemovalCause.COLLECTED;
} else if (hasExpired(node, expirationTicker().read())) {
cause = RemovalCause.EXPIRED;
} else {
cause = RemovalCause.EXPLICIT;
}

if (hasRemovalListener()) {
@SuppressWarnings("unchecked")
K castKey = (K) key;
notifyRemoval(castKey, oldValue, cause);
}
afterWrite(new RemovalTask(node));
return (cause == RemovalCause.EXPLICIT) ? oldValue : null;
}

/**
* Removes the mapping for a key after notifying the writer.
*
* @param key key whose mapping is to be removed
* @return the removed value or null if no mapping was found
*/
@Nullable V removeWithWriter(Object key) {
@SuppressWarnings("unchecked")
K castKey = (K) key;
@SuppressWarnings({"unchecked", "rawtypes"})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ default Map<K, V> getAll(@NonNull Iterable<? extends @NonNull K> keys,
* @param keys the keys whose associated values are to be removed
* @throws NullPointerException if the specified collection is null or contains a null element
*/
void invalidateAll(@NonNull Iterable<?> keys);
void invalidateAll(@NonNull Iterable<@NonNull ?> keys);

/**
* Discards all entries in the cache. The behavior of this operation is undefined for an entry
Expand Down
Loading

0 comments on commit 8121c1d

Please sign in to comment.