Repository: groovy Updated Branches: refs/heads/master 59f619b9d -> 8a85833ff
Revert "Improve the performance of `ConcurrentCommonCache` further(3.0.0+ Only)" StampedLocks are not reentrant, so it may cause deadlock: readlock -> writelock -> readlock(fails to get), See: ClosuresSpecTest.testMemoize Project: http://git-wip-us.apache.org/repos/asf/groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/8a85833f Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/8a85833f Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/8a85833f Branch: refs/heads/master Commit: 8a85833ff15c90c93bd1693de450c039cda4e068 Parents: 59f619b Author: danielsun1106 <[email protected]> Authored: Sun Mar 4 11:55:59 2018 +0800 Committer: danielsun1106 <[email protected]> Committed: Sun Mar 4 11:58:44 2018 +0800 ---------------------------------------------------------------------- .../runtime/memoize/ConcurrentCommonCache.java | 75 +++++++------------- .../memoize/ConcurrentCommonCacheTest.java | 38 ---------- 2 files changed, 25 insertions(+), 88 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/groovy/blob/8a85833f/src/main/java/org/codehaus/groovy/runtime/memoize/ConcurrentCommonCache.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/runtime/memoize/ConcurrentCommonCache.java b/src/main/java/org/codehaus/groovy/runtime/memoize/ConcurrentCommonCache.java index 79b622d..27ee102 100644 --- a/src/main/java/org/codehaus/groovy/runtime/memoize/ConcurrentCommonCache.java +++ b/src/main/java/org/codehaus/groovy/runtime/memoize/ConcurrentCommonCache.java @@ -23,7 +23,7 @@ import java.io.Serializable; import java.util.Collection; import java.util.Map; import java.util.Set; -import java.util.concurrent.locks.StampedLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; /** * Represents a simple key-value cache, which is thread safe and backed by a {@link java.util.Map} instance @@ -36,7 +36,9 @@ import java.util.concurrent.locks.StampedLock; public class ConcurrentCommonCache<K, V> implements EvictableCache<K, V>, ValueConvertable<V, Object>, Serializable { private static final long serialVersionUID = -7352338549333024936L; - private final StampedLock sl = new StampedLock(); + private final ReentrantReadWriteLock rwl = new ReentrantReadWriteLock(); + private final ReentrantReadWriteLock.ReadLock readLock = rwl.readLock(); + private final ReentrantReadWriteLock.WriteLock writeLock = rwl.writeLock(); private final CommonCache<K, V> commonCache; /** @@ -114,55 +116,35 @@ public class ConcurrentCommonCache<K, V> implements EvictableCache<K, V>, ValueC public V getAndPut(K key, ValueProvider<? super K, ? extends V> valueProvider, boolean shouldCache) { V value; - // try optimistic read first, which is non-blocking - long optimisticReadStamp = sl.tryOptimisticRead(); - value = commonCache.get(key); - if (sl.validate(optimisticReadStamp)) { + readLock.lock(); + try { + value = commonCache.get(key); if (null != convertValue(value)) { return value; } + } finally { + readLock.unlock(); } - long stamp = sl.readLock(); + writeLock.lock(); try { - // if stale, read again - if (!sl.validate(optimisticReadStamp)) { - value = commonCache.get(key); - if (null != convertValue(value)) { - return value; - } + // try to find the cached value again + value = commonCache.get(key); + if (null != convertValue(value)) { + return value; } - long ws = sl.tryConvertToWriteLock(stamp); // the new local variable `ws` is necessary here! - if (0L == ws) { // Failed to convert read lock to write lock - sl.unlockRead(stamp); - stamp = sl.writeLock(); - - // try to read again - value = commonCache.get(key); - if (null != convertValue(value)) { - return value; - } - } else { - stamp = ws; + value = null == valueProvider ? null : valueProvider.provide(key); + if (shouldCache && null != convertValue(value)) { + commonCache.put(key, value); } - - value = compute(key, valueProvider, shouldCache); } finally { - sl.unlock(stamp); + writeLock.unlock(); } return value; } - private V compute(K key, ValueProvider<? super K, ? extends V> valueProvider, boolean shouldCache) { - V value = null == valueProvider ? null : valueProvider.provide(key); - if (shouldCache && null != convertValue(value)) { - commonCache.put(key, value); - } - return value; - } - /** * {@inheritDoc} */ @@ -235,11 +217,11 @@ public class ConcurrentCommonCache<K, V> implements EvictableCache<K, V>, ValueC * @param action the content to complete */ public <R> R doWithWriteLock(Action<K, V, R> action) { - long stamp = sl.writeLock(); + writeLock.lock(); try { return action.doWith(commonCache); } finally { - sl.unlockWrite(stamp); + writeLock.unlock(); } } @@ -248,19 +230,12 @@ public class ConcurrentCommonCache<K, V> implements EvictableCache<K, V>, ValueC * @param action the content to complete */ public <R> R doWithReadLock(Action<K, V, R> action) { - long stamp = sl.tryOptimisticRead(); - R result = action.doWith(commonCache); - - if (!sl.validate(stamp)) { - stamp = sl.readLock(); - try { - result = action.doWith(commonCache); - } finally { - sl.unlockRead(stamp); - } + readLock.lock(); + try { + return action.doWith(commonCache); + } finally { + readLock.unlock(); } - - return result; } @FunctionalInterface http://git-wip-us.apache.org/repos/asf/groovy/blob/8a85833f/src/test/org/codehaus/groovy/runtime/memoize/ConcurrentCommonCacheTest.java ---------------------------------------------------------------------- diff --git a/src/test/org/codehaus/groovy/runtime/memoize/ConcurrentCommonCacheTest.java b/src/test/org/codehaus/groovy/runtime/memoize/ConcurrentCommonCacheTest.java index d012e99..44c9ae2 100644 --- a/src/test/org/codehaus/groovy/runtime/memoize/ConcurrentCommonCacheTest.java +++ b/src/test/org/codehaus/groovy/runtime/memoize/ConcurrentCommonCacheTest.java @@ -19,19 +19,11 @@ package org.codehaus.groovy.runtime.memoize; import org.apache.groovy.util.Maps; -import org.apache.groovy.util.concurrentlinkedhashmap.ConcurrentLinkedHashMap; import org.junit.Assert; import org.junit.Test; import java.util.HashMap; import java.util.LinkedHashMap; -import java.util.TreeSet; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.atomic.AtomicInteger; - -import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotEquals; public class ConcurrentCommonCacheTest { @Test @@ -208,34 +200,4 @@ public class ConcurrentCommonCacheTest { Assert.assertEquals("3", sc.get("c")); Assert.assertEquals("5", sc.get("d")); } - - @Test - public void testAccessCacheConcurrently() throws InterruptedException { - final ConcurrentCommonCache<Integer, Integer> m = new ConcurrentCommonCache<>(); - - final int threadNum = 60; - final CountDownLatch countDownLatch = new CountDownLatch(1); - final CountDownLatch countDownLatch2 = new CountDownLatch(threadNum); - - final AtomicInteger cnt = new AtomicInteger(0); - - for (int i = 0; i < threadNum; i++) { - new Thread(() -> { - try { - countDownLatch.await(); - - m.getAndPut(123, k -> cnt.getAndIncrement()); - } catch (InterruptedException e) { - e.printStackTrace(); - } finally { - countDownLatch2.countDown(); - } - }).start(); - } - - countDownLatch.countDown(); - countDownLatch2.await(); - - Assert.assertEquals(1, cnt.get()); - } }
