From dc7d30d08eacbe4d00d16b13e921359d38c77cd8 Mon Sep 17 00:00:00 2001 From: Doug Lea Date: Fri, 1 Nov 2019 09:04:04 -0700 Subject: [PATCH] 8231592: Clarify that ConcurrentHashMap compute methods mapping functions execute at most once Reviewed-by: martin --- .../util/concurrent/ConcurrentHashMap.java | 34 +++++---- .../concurrent/tck/ConcurrentHashMapTest.java | 2 - .../tck/ConcurrentSkipListMapTest.java | 3 +- .../java/util/concurrent/tck/HashMapTest.java | 2 - .../util/concurrent/tck/HashtableTest.java | 2 - .../util/concurrent/tck/JSR166TestCase.java | 9 ++- .../concurrent/tck/LinkedHashMapTest.java | 2 - .../concurrent/tck/MapImplementation.java | 10 ++- .../jdk/java/util/concurrent/tck/MapTest.java | 73 ++++++++++++++++++- .../java/util/concurrent/tck/TreeMapTest.java | 2 - 10 files changed, 109 insertions(+), 30 deletions(-) diff --git a/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java b/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java index c5001de468e..117bb72a1a6 100644 --- a/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java +++ b/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java @@ -1667,11 +1667,14 @@ public class ConcurrentHashMap extends AbstractMap * If the specified key is not already associated with a value, * attempts to compute its value using the given mapping function * and enters it into this map unless {@code null}. The entire - * method invocation is performed atomically, so the function is - * applied at most once per key. Some attempted update operations - * on this map by other threads may be blocked while computation - * is in progress, so the computation should be short and simple, - * and must not attempt to update any other mappings of this map. + * method invocation is performed atomically. The supplied + * function is invoked exactly once per invocation of this method + * if the key is absent, else not at all. Some attempted update + * operations on this map by other threads may be blocked while + * computation is in progress, so the computation should be short + * and simple. + * + *

The mapping function must not modify this map during computation. * * @param key key with which the specified value is to be associated * @param mappingFunction the function to compute a value @@ -1778,10 +1781,13 @@ public class ConcurrentHashMap extends AbstractMap * If the value for the specified key is present, attempts to * compute a new mapping given the key and its current mapped * value. The entire method invocation is performed atomically. - * Some attempted update operations on this map by other threads - * may be blocked while computation is in progress, so the - * computation should be short and simple, and must not attempt to - * update any other mappings of this map. + * The supplied function is invoked exactly once per invocation of + * this method if the key is present, else not at all. Some + * attempted update operations on this map by other threads may be + * blocked while computation is in progress, so the computation + * should be short and simple. + * + *

The remapping function must not modify this map during computation. * * @param key key with which a value may be associated * @param remappingFunction the function to compute a value @@ -1870,10 +1876,12 @@ public class ConcurrentHashMap extends AbstractMap * Attempts to compute a mapping for the specified key and its * current mapped value (or {@code null} if there is no current * mapping). The entire method invocation is performed atomically. - * Some attempted update operations on this map by other threads - * may be blocked while computation is in progress, so the - * computation should be short and simple, and must not attempt to - * update any other mappings of this Map. + * The supplied function is invoked exactly once per invocation of + * this method. Some attempted update operations on this map by + * other threads may be blocked while computation is in progress, + * so the computation should be short and simple. + * + *

The remapping function must not modify this map during computation. * * @param key key with which the specified value is to be associated * @param remappingFunction the function to compute a value diff --git a/test/jdk/java/util/concurrent/tck/ConcurrentHashMapTest.java b/test/jdk/java/util/concurrent/tck/ConcurrentHashMapTest.java index 33df45b17ba..8cd55c9b746 100644 --- a/test/jdk/java/util/concurrent/tck/ConcurrentHashMapTest.java +++ b/test/jdk/java/util/concurrent/tck/ConcurrentHashMapTest.java @@ -55,8 +55,6 @@ public class ConcurrentHashMapTest extends JSR166TestCase { class Implementation implements MapImplementation { public Class klazz() { return ConcurrentHashMap.class; } public Map emptyMap() { return new ConcurrentHashMap(); } - public Object makeKey(int i) { return i; } - public Object makeValue(int i) { return i; } public boolean isConcurrent() { return true; } public boolean permitsNullKeys() { return false; } public boolean permitsNullValues() { return false; } diff --git a/test/jdk/java/util/concurrent/tck/ConcurrentSkipListMapTest.java b/test/jdk/java/util/concurrent/tck/ConcurrentSkipListMapTest.java index 376f1f7ebb8..1297186b61d 100644 --- a/test/jdk/java/util/concurrent/tck/ConcurrentSkipListMapTest.java +++ b/test/jdk/java/util/concurrent/tck/ConcurrentSkipListMapTest.java @@ -54,9 +54,8 @@ public class ConcurrentSkipListMapTest extends JSR166TestCase { class Implementation implements MapImplementation { public Class klazz() { return ConcurrentSkipListMap.class; } public Map emptyMap() { return new ConcurrentSkipListMap(); } - public Object makeKey(int i) { return i; } - public Object makeValue(int i) { return i; } public boolean isConcurrent() { return true; } + public boolean remappingFunctionCalledAtMostOnce() { return false; }; public boolean permitsNullKeys() { return false; } public boolean permitsNullValues() { return false; } public boolean supportsSetValue() { return false; } diff --git a/test/jdk/java/util/concurrent/tck/HashMapTest.java b/test/jdk/java/util/concurrent/tck/HashMapTest.java index 357d28038bb..19abb0eee15 100644 --- a/test/jdk/java/util/concurrent/tck/HashMapTest.java +++ b/test/jdk/java/util/concurrent/tck/HashMapTest.java @@ -46,8 +46,6 @@ public class HashMapTest extends JSR166TestCase { class Implementation implements MapImplementation { public Class klazz() { return HashMap.class; } public Map emptyMap() { return new HashMap(); } - public Object makeKey(int i) { return i; } - public Object makeValue(int i) { return i; } public boolean isConcurrent() { return false; } public boolean permitsNullKeys() { return true; } public boolean permitsNullValues() { return true; } diff --git a/test/jdk/java/util/concurrent/tck/HashtableTest.java b/test/jdk/java/util/concurrent/tck/HashtableTest.java index c8bf780921f..dc8bed1d146 100644 --- a/test/jdk/java/util/concurrent/tck/HashtableTest.java +++ b/test/jdk/java/util/concurrent/tck/HashtableTest.java @@ -44,8 +44,6 @@ public class HashtableTest extends JSR166TestCase { class Implementation implements MapImplementation { public Class klazz() { return Hashtable.class; } public Map emptyMap() { return new Hashtable(); } - public Object makeKey(int i) { return i; } - public Object makeValue(int i) { return i; } public boolean isConcurrent() { return true; } public boolean permitsNullKeys() { return false; } public boolean permitsNullValues() { return false; } diff --git a/test/jdk/java/util/concurrent/tck/JSR166TestCase.java b/test/jdk/java/util/concurrent/tck/JSR166TestCase.java index c52e5507a3a..f10204afa58 100644 --- a/test/jdk/java/util/concurrent/tck/JSR166TestCase.java +++ b/test/jdk/java/util/concurrent/tck/JSR166TestCase.java @@ -728,6 +728,13 @@ public class JSR166TestCase extends TestCase { return ThreadLocalRandom.current().nextBoolean(); } + /** + * Returns a random element from given choices. + */ + T chooseRandomly(List choices) { + return choices.get(ThreadLocalRandom.current().nextInt(choices.size())); + } + /** * Returns a random element from given choices. */ @@ -1799,7 +1806,7 @@ public class JSR166TestCase extends TestCase { public int await() { try { - return super.await(2 * LONG_DELAY_MS, MILLISECONDS); + return super.await(LONGER_DELAY_MS, MILLISECONDS); } catch (TimeoutException timedOut) { throw new AssertionError("timed out"); } catch (Exception fail) { diff --git a/test/jdk/java/util/concurrent/tck/LinkedHashMapTest.java b/test/jdk/java/util/concurrent/tck/LinkedHashMapTest.java index c572d403b36..16b4ba7a117 100644 --- a/test/jdk/java/util/concurrent/tck/LinkedHashMapTest.java +++ b/test/jdk/java/util/concurrent/tck/LinkedHashMapTest.java @@ -46,8 +46,6 @@ public class LinkedHashMapTest extends JSR166TestCase { class Implementation implements MapImplementation { public Class klazz() { return LinkedHashMap.class; } public Map emptyMap() { return new LinkedHashMap(); } - public Object makeKey(int i) { return i; } - public Object makeValue(int i) { return i; } public boolean isConcurrent() { return false; } public boolean permitsNullKeys() { return true; } public boolean permitsNullValues() { return true; } diff --git a/test/jdk/java/util/concurrent/tck/MapImplementation.java b/test/jdk/java/util/concurrent/tck/MapImplementation.java index eba9a6609da..f7b83968068 100644 --- a/test/jdk/java/util/concurrent/tck/MapImplementation.java +++ b/test/jdk/java/util/concurrent/tck/MapImplementation.java @@ -40,9 +40,15 @@ public interface MapImplementation { public Class klazz(); /** Returns an empty map. */ public Map emptyMap(); - public Object makeKey(int i); - public Object makeValue(int i); + + // General purpose implementations can use Integers for key and value + default Object makeKey(int i) { return i; } + default Object makeValue(int i) { return i; } + default int keyToInt(Object key) { return (Integer) key; } + default int valueToInt(Object value) { return (Integer) value; } + public boolean isConcurrent(); + default boolean remappingFunctionCalledAtMostOnce() { return true; }; public boolean permitsNullKeys(); public boolean permitsNullValues(); public boolean supportsSetValue(); diff --git a/test/jdk/java/util/concurrent/tck/MapTest.java b/test/jdk/java/util/concurrent/tck/MapTest.java index 9d6cedf0f05..3a37624daa3 100644 --- a/test/jdk/java/util/concurrent/tck/MapTest.java +++ b/test/jdk/java/util/concurrent/tck/MapTest.java @@ -32,13 +32,17 @@ * http://creativecommons.org/publicdomain/zero/1.0/ */ -import junit.framework.Test; - import java.util.ArrayList; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; +import java.util.function.BiFunction; + +import junit.framework.Test; /** * Contains tests applicable to all Map implementations. @@ -227,6 +231,71 @@ public class MapTest extends JSR166TestCase { assertTrue(clone.isEmpty()); } + /** + * Concurrent access by compute methods behaves as expected + */ + public void testConcurrentAccess() throws Throwable { + final Map map = impl.emptyMap(); + final long testDurationMillis = expensiveTests ? 1000 : 2; + final int nTasks = impl.isConcurrent() + ? ThreadLocalRandom.current().nextInt(1, 10) + : 1; + final AtomicBoolean done = new AtomicBoolean(false); + final boolean remappingFunctionCalledAtMostOnce + = impl.remappingFunctionCalledAtMostOnce(); + final List futures = new ArrayList<>(); + final AtomicLong expectedSum = new AtomicLong(0); + final Action[] tasks = { + // repeatedly increment values using compute() + () -> { + long[] invocations = new long[2]; + ThreadLocalRandom rnd = ThreadLocalRandom.current(); + BiFunction incValue = (k, v) -> { + invocations[1]++; + int vi = (v == null) ? 1 : impl.valueToInt(v) + 1; + return impl.makeValue(vi); + }; + while (!done.getAcquire()) { + invocations[0]++; + Object key = impl.makeKey(3 * rnd.nextInt(10)); + map.compute(key, incValue); + } + if (remappingFunctionCalledAtMostOnce) + assertEquals(invocations[0], invocations[1]); + expectedSum.getAndAdd(invocations[0]); + }, + // repeatedly increment values using computeIfPresent() + () -> { + long[] invocations = new long[2]; + ThreadLocalRandom rnd = ThreadLocalRandom.current(); + BiFunction incValue = (k, v) -> { + invocations[1]++; + int vi = impl.valueToInt(v) + 1; + return impl.makeValue(vi); + }; + while (!done.getAcquire()) { + Object key = impl.makeKey(3 * rnd.nextInt(10)); + if (map.computeIfPresent(key, incValue) != null) + invocations[0]++; + } + if (remappingFunctionCalledAtMostOnce) + assertEquals(invocations[0], invocations[1]); + expectedSum.getAndAdd(invocations[0]); + }, + }; + for (int i = nTasks; i--> 0; ) { + Action task = chooseRandomly(tasks); + futures.add(CompletableFuture.runAsync(checkedRunnable(task))); + } + Thread.sleep(testDurationMillis); + done.setRelease(true); + for (var future : futures) + checkTimedGet(future, null); + + long sum = map.values().stream().mapToLong(x -> (int) x).sum(); + assertEquals(expectedSum.get(), sum); + } + // public void testFailsIntentionallyForDebugging() { // fail(impl.klazz().getSimpleName()); // } diff --git a/test/jdk/java/util/concurrent/tck/TreeMapTest.java b/test/jdk/java/util/concurrent/tck/TreeMapTest.java index ca9c544f9fa..73e1d3e9efe 100644 --- a/test/jdk/java/util/concurrent/tck/TreeMapTest.java +++ b/test/jdk/java/util/concurrent/tck/TreeMapTest.java @@ -53,8 +53,6 @@ public class TreeMapTest extends JSR166TestCase { class Implementation implements MapImplementation { public Class klazz() { return TreeMap.class; } public Map emptyMap() { return new TreeMap(); } - public Object makeKey(int i) { return i; } - public Object makeValue(int i) { return i; } public boolean isConcurrent() { return false; } public boolean permitsNullKeys() { return false; } public boolean permitsNullValues() { return true; }