Repository: groovy Updated Branches: refs/heads/GROOVY_2_5_X 4629ba7a8 -> 785d69703
Refine "GROOVY-8486 - Closure executed multiple times even if memoized" (cherry picked from commit ac6f3b0) Project: http://git-wip-us.apache.org/repos/asf/groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/785d6970 Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/785d6970 Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/785d6970 Branch: refs/heads/GROOVY_2_5_X Commit: 785d697038f1261c5b7ade9452a2e1d91477049a Parents: 4629ba7 Author: sunlan <sun...@apache.org> Authored: Fri Mar 2 15:53:30 2018 +0800 Committer: sunlan <sun...@apache.org> Committed: Fri Mar 2 16:18:47 2018 +0800 ---------------------------------------------------------------------- src/main/groovy/groovy/lang/Closure.java | 5 +- .../groovy/runtime/memoize/CommonCache.java | 14 ++- .../runtime/memoize/ConcurrentCommonCache.java | 46 +++++++++- .../runtime/memoize/ConcurrentSoftCache.java | 94 ++++++++++++++++++++ .../runtime/memoize/LRUProtectionStorage.java | 2 +- .../groovy/runtime/memoize/Memoize.java | 41 +++++---- .../runtime/memoize/ProtectionStorage.java | 4 +- .../runtime/memoize/ValueConvertable.java | 36 ++++++++ .../runtime/memoize/MemoizeAtLeastTest.groovy | 14 +++ .../runtime/memoize/MemoizeAtMostTest.groovy | 14 +++ .../runtime/memoize/MemoizeBetweenTest.groovy | 14 +++ 11 files changed, 256 insertions(+), 28 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/groovy/blob/785d6970/src/main/groovy/groovy/lang/Closure.java ---------------------------------------------------------------------- diff --git a/src/main/groovy/groovy/lang/Closure.java b/src/main/groovy/groovy/lang/Closure.java index 13e3cf2..11e2611 100644 --- a/src/main/groovy/groovy/lang/Closure.java +++ b/src/main/groovy/groovy/lang/Closure.java @@ -28,6 +28,7 @@ import org.codehaus.groovy.runtime.InvokerHelper; import org.codehaus.groovy.runtime.InvokerInvocationException; import org.codehaus.groovy.runtime.callsite.BooleanClosureWrapper; import org.codehaus.groovy.runtime.memoize.ConcurrentCommonCache; +import org.codehaus.groovy.runtime.memoize.ConcurrentSoftCache; import org.codehaus.groovy.runtime.memoize.LRUCache; import org.codehaus.groovy.runtime.memoize.Memoize; @@ -750,7 +751,7 @@ public abstract class Closure<V> extends GroovyObjectSupport implements Cloneabl public Closure<V> memoizeAtLeast(final int protectedCacheSize) { if (protectedCacheSize < 0) throw new IllegalArgumentException("A non-negative number is required as the protectedCacheSize parameter for memoizeAtLeast."); - return Memoize.buildSoftReferenceMemoizeFunction(protectedCacheSize, new ConcurrentCommonCache(), this); + return Memoize.buildSoftReferenceMemoizeFunction(protectedCacheSize, new ConcurrentSoftCache<Object, Object>(), this); } /** @@ -782,7 +783,7 @@ public abstract class Closure<V> extends GroovyObjectSupport implements Cloneabl if (maxCacheSize < 0) throw new IllegalArgumentException("A non-negative number is required as the maxCacheSize parameter for memoizeBetween."); if (protectedCacheSize > maxCacheSize) throw new IllegalArgumentException("The maxCacheSize parameter to memoizeBetween is required to be greater or equal to the protectedCacheSize parameter."); - return Memoize.buildSoftReferenceMemoizeFunction(protectedCacheSize, new LRUCache(maxCacheSize), this); + return Memoize.buildSoftReferenceMemoizeFunction(protectedCacheSize, new ConcurrentSoftCache<Object, Object>(maxCacheSize), this); } /** http://git-wip-us.apache.org/repos/asf/groovy/blob/785d6970/src/main/java/org/codehaus/groovy/runtime/memoize/CommonCache.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/runtime/memoize/CommonCache.java b/src/main/java/org/codehaus/groovy/runtime/memoize/CommonCache.java index 29f048f..af193da 100644 --- a/src/main/java/org/codehaus/groovy/runtime/memoize/CommonCache.java +++ b/src/main/java/org/codehaus/groovy/runtime/memoize/CommonCache.java @@ -36,7 +36,7 @@ import java.util.Set; * @param <V> type of the values * @since 2.5.0 */ -public class CommonCache<K, V> implements EvictableCache<K, V>, Serializable { +public class CommonCache<K, V> implements EvictableCache<K, V>, ValueConvertable<V, Object>, Serializable { private static final long serialVersionUID = 934699400232698324L; /** * The default load factor @@ -130,12 +130,12 @@ public class CommonCache<K, V> implements EvictableCache<K, V>, Serializable { public V getAndPut(K key, ValueProvider<? super K, ? extends V> valueProvider, boolean shouldCache) { V value = get(key); - if (null != value) { + if (null != convertValue(value)) { return value; } value = null == valueProvider ? null : valueProvider.provide(key); - if (shouldCache && null != value) { + if (shouldCache && null != convertValue(value)) { put(key, value); } @@ -219,4 +219,12 @@ public class CommonCache<K, V> implements EvictableCache<K, V>, Serializable { public String toString() { return map.toString(); } + + /** + * {@inheritDoc} + */ + @Override + public Object convertValue(V value) { + return value; + } } http://git-wip-us.apache.org/repos/asf/groovy/blob/785d6970/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 f0a1e1c..4bbfc9b 100644 --- a/src/main/java/org/codehaus/groovy/runtime/memoize/ConcurrentCommonCache.java +++ b/src/main/java/org/codehaus/groovy/runtime/memoize/ConcurrentCommonCache.java @@ -33,7 +33,7 @@ import java.util.concurrent.locks.ReentrantReadWriteLock; * @since 2.5.0 */ @ThreadSafe -public class ConcurrentCommonCache<K, V> implements EvictableCache<K, V>, Serializable { +public class ConcurrentCommonCache<K, V> implements EvictableCache<K, V>, ValueConvertable<V, Object>, Serializable { private static final long serialVersionUID = -7352338549333024936L; private final ReentrantReadWriteLock rwl = new ReentrantReadWriteLock(); @@ -129,7 +129,7 @@ public class ConcurrentCommonCache<K, V> implements EvictableCache<K, V>, Serial readLock.lock(); try { value = commonCache.get(key); - if (null != value) { + if (null != convertValue(value)) { return value; } } finally { @@ -140,12 +140,12 @@ public class ConcurrentCommonCache<K, V> implements EvictableCache<K, V>, Serial try { // try to find the cached value again value = commonCache.get(key); - if (null != value) { + if (null != convertValue(value)) { return value; } value = null == valueProvider ? null : valueProvider.provide(key); - if (shouldCache && null != value) { + if (shouldCache && null != convertValue(value)) { commonCache.put(key, value); } } finally { @@ -245,4 +245,42 @@ public class ConcurrentCommonCache<K, V> implements EvictableCache<K, V>, Serial writeLock.unlock(); } } + + /** + * {@inheritDoc} + */ + @Override + public Object convertValue(V value) { + return value; + } + + /** + * deal with the backed cache guarded by write lock + * @param action the content to complete + */ + public <R> R doWithWriteLock(Action<K, V, R> action) { + writeLock.lock(); + try { + return action.doWith(commonCache); + } finally { + writeLock.unlock(); + } + } + + /** + * deal with the backed cache guarded by read lock + * @param action the content to complete + */ + public <R> R doWithReadLock(Action<K, V, R> action) { + readLock.lock(); + try { + return action.doWith(commonCache); + } finally { + readLock.unlock(); + } + } + + public interface Action<K, V, R> { + R doWith(CommonCache<K, V> commonCache); + } } http://git-wip-us.apache.org/repos/asf/groovy/blob/785d6970/src/main/java/org/codehaus/groovy/runtime/memoize/ConcurrentSoftCache.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/runtime/memoize/ConcurrentSoftCache.java b/src/main/java/org/codehaus/groovy/runtime/memoize/ConcurrentSoftCache.java new file mode 100644 index 0000000..a265810 --- /dev/null +++ b/src/main/java/org/codehaus/groovy/runtime/memoize/ConcurrentSoftCache.java @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.codehaus.groovy.runtime.memoize; + +import java.lang.ref.SoftReference; +import java.util.Map; + +/** + * Represents concurrent cache holding SoftReference instance as value + * + * @param <K> key type + * @param <V> real value type + */ +public class ConcurrentSoftCache<K, V> extends ConcurrentCommonCache<K, SoftReference<V>> { + private static final long serialVersionUID = 5646536868666351819L; + + + /** + * Constructs a cache with unlimited size + */ + public ConcurrentSoftCache() { + super(); + } + + /** + * Constructs a cache with limited size + * + * @param initialCapacity initial capacity of the cache + * @param maxSize max size of the cache + * @param evictionStrategy LRU or FIFO, see {@link org.codehaus.groovy.runtime.memoize.EvictableCache.EvictionStrategy} + */ + public ConcurrentSoftCache(int initialCapacity, int maxSize, EvictionStrategy evictionStrategy) { + super(initialCapacity, maxSize, evictionStrategy); + } + + /** + * Constructs a LRU cache with the specified initial capacity and max size. + * The LRU cache is slower than {@link LRUCache} + * + * @param initialCapacity initial capacity of the LRU cache + * @param maxSize max size of the LRU cache + */ + public ConcurrentSoftCache(int initialCapacity, int maxSize) { + super(initialCapacity, maxSize); + } + + /** + * Constructs a LRU cache with the default initial capacity(16) + * + * @param maxSize max size of the LRU cache + * @see #ConcurrentSoftCache(int, int) + */ + public ConcurrentSoftCache(int maxSize) { + super(maxSize); + } + + /** + * Constructs a cache backed by the specified {@link java.util.Map} instance + * + * @param map the {@link java.util.Map} instance + */ + public ConcurrentSoftCache(Map<K, SoftReference<V>> map) { + super(map); + } + + /** + * {@inheritDoc} + */ + @Override + public Object convertValue(SoftReference<V> value) { + if (null == value) { + return null; + } + + return value.get(); + } +} http://git-wip-us.apache.org/repos/asf/groovy/blob/785d6970/src/main/java/org/codehaus/groovy/runtime/memoize/LRUProtectionStorage.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/runtime/memoize/LRUProtectionStorage.java b/src/main/java/org/codehaus/groovy/runtime/memoize/LRUProtectionStorage.java index 1a1e65d..b7b0637 100644 --- a/src/main/java/org/codehaus/groovy/runtime/memoize/LRUProtectionStorage.java +++ b/src/main/java/org/codehaus/groovy/runtime/memoize/LRUProtectionStorage.java @@ -42,7 +42,7 @@ final class LRUProtectionStorage extends LinkedHashMap<Object, Object> implement * The eldest entry should be removed when we reached the maximum cache size */ @Override - protected boolean removeEldestEntry(final Map.Entry<Object, Object> eldest) { + protected synchronized boolean removeEldestEntry(final Map.Entry<Object, Object> eldest) { return size() > maxSize; } http://git-wip-us.apache.org/repos/asf/groovy/blob/785d6970/src/main/java/org/codehaus/groovy/runtime/memoize/Memoize.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/runtime/memoize/Memoize.java b/src/main/java/org/codehaus/groovy/runtime/memoize/Memoize.java index 98c1bb7..035aee7 100644 --- a/src/main/java/org/codehaus/groovy/runtime/memoize/Memoize.java +++ b/src/main/java/org/codehaus/groovy/runtime/memoize/Memoize.java @@ -76,7 +76,7 @@ public abstract class Memoize { * @param <V> The closure's return type * @return A new memoized closure */ - public static <V> Closure<V> buildSoftReferenceMemoizeFunction(final int protectedCacheSize, final MemoizeCache<Object, Object> cache, final Closure<V> closure) { + public static <V> Closure<V> buildSoftReferenceMemoizeFunction(final int protectedCacheSize, final MemoizeCache<Object, SoftReference<Object>> cache, final Closure<V> closure) { final ProtectionStorage lruProtectionStorage = protectedCacheSize > 0 ? new LRUProtectionStorage(protectedCacheSize) : new NullProtectionStorage(); // Nothing should be done when no elements need protection against eviction @@ -120,15 +120,20 @@ public abstract class Memoize { final MemoizeCache<Object, Object> cache; final Closure<V> closure; - MemoizeFunction(final MemoizeCache<Object, Object> cache, Closure<V> closure) { + MemoizeFunction(final MemoizeCache<Object, ?> cache, Closure<V> closure) { super(closure.getOwner()); - this.cache = cache; + this.cache = coerce(cache); this.closure = closure; parameterTypes = closure.getParameterTypes(); maximumNumberOfParameters = closure.getMaximumNumberOfParameters(); } - - @Override public V call(final Object... args) { + + private static MemoizeCache coerce(MemoizeCache<Object, ?> cache) { + return cache; + } + + @Override + public V call(final Object... args) { final Object key = generateKey(args); Object result = cache.getAndPut(key, new MemoizeCache.ValueProvider<Object, Object>() { @Override @@ -152,26 +157,30 @@ public abstract class Memoize { final ProtectionStorage lruProtectionStorage; final ReferenceQueue queue; - SoftReferenceMemoizeFunction(final MemoizeCache<Object, Object> cache, Closure<V> closure, + SoftReferenceMemoizeFunction(final MemoizeCache<Object, SoftReference<Object>> cache, Closure<V> closure, ProtectionStorage lruProtectionStorage, ReferenceQueue queue) { super(cache, closure); this.lruProtectionStorage = lruProtectionStorage; this.queue = queue; } - - @Override public V call(final Object... args) { + + @Override + public V call(final Object... args) { if (queue.poll() != null) cleanUpNullReferences(cache, queue); // if something has been evicted, do a clean-up final Object key = generateKey(args); - final SoftReference reference = (SoftReference) cache.get(key); - Object result = reference != null ? reference.get() : null; - if (result == null) { - result = closure.call(args); - if (result == null) { - result = MEMOIZE_NULL; + + SoftReference reference = (SoftReference) cache.getAndPut(key, new MemoizeCache.ValueProvider<Object, Object>() { + @Override + public Object provide(Object key) { + Object r = closure.call(args); + + return null != r ? new SoftReference<Object>(r, queue) : new SoftReference<Object>(MEMOIZE_NULL); } - cache.put(key, new SoftReference(result, queue)); - } + }); + + Object result = reference.get(); lruProtectionStorage.touch(key, result); + return result == MEMOIZE_NULL ? null : (V) result; } http://git-wip-us.apache.org/repos/asf/groovy/blob/785d6970/src/main/java/org/codehaus/groovy/runtime/memoize/ProtectionStorage.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/runtime/memoize/ProtectionStorage.java b/src/main/java/org/codehaus/groovy/runtime/memoize/ProtectionStorage.java index 31159a4..a820389 100644 --- a/src/main/java/org/codehaus/groovy/runtime/memoize/ProtectionStorage.java +++ b/src/main/java/org/codehaus/groovy/runtime/memoize/ProtectionStorage.java @@ -24,6 +24,6 @@ package org.codehaus.groovy.runtime.memoize; * * @author Vaclav Pech */ -interface ProtectionStorage { - void touch(Object key, Object value); +interface ProtectionStorage<K, V> { + void touch(K key, V value); } http://git-wip-us.apache.org/repos/asf/groovy/blob/785d6970/src/main/java/org/codehaus/groovy/runtime/memoize/ValueConvertable.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/runtime/memoize/ValueConvertable.java b/src/main/java/org/codehaus/groovy/runtime/memoize/ValueConvertable.java new file mode 100644 index 0000000..3f3f692 --- /dev/null +++ b/src/main/java/org/codehaus/groovy/runtime/memoize/ValueConvertable.java @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.codehaus.groovy.runtime.memoize; + +/** + * To support caches whose values are convertable, e.g. SoftReference, WeakReference + * + * @param <V1> source value type, e.g. SoftReference, WeakReference + * @param <V2> target value type, e.g. value that SoftReference or WeakReference referenced + */ +public interface ValueConvertable<V1, V2> { + /** + * convert the original value to the target value + * + * @param value the original value + * @return the converted value + */ + V2 convertValue(V1 value); +} http://git-wip-us.apache.org/repos/asf/groovy/blob/785d6970/src/test/org/codehaus/groovy/runtime/memoize/MemoizeAtLeastTest.groovy ---------------------------------------------------------------------- diff --git a/src/test/org/codehaus/groovy/runtime/memoize/MemoizeAtLeastTest.groovy b/src/test/org/codehaus/groovy/runtime/memoize/MemoizeAtLeastTest.groovy index 2535b81..dd1dda8 100644 --- a/src/test/org/codehaus/groovy/runtime/memoize/MemoizeAtLeastTest.groovy +++ b/src/test/org/codehaus/groovy/runtime/memoize/MemoizeAtLeastTest.groovy @@ -18,6 +18,8 @@ */ package org.codehaus.groovy.runtime.memoize +import java.util.concurrent.atomic.AtomicInteger + /** * @author Vaclav Pech */ @@ -37,4 +39,16 @@ public class MemoizeAtLeastTest extends AbstractMemoizeTestCase { [1, 2, 3, 4, 5, 6].each {mem(it)} assert flag } + + public void testMemoizeAtLeastConcurrently() { + AtomicInteger cnt = new AtomicInteger(0) + Closure cl = { + cnt.incrementAndGet() + it * 2 + } + Closure mem = cl.memoizeAtLeast(3) + [4, 5, 6, 4, 5, 6, 4, 5, 6].collect { num -> Thread.start { mem(num) } }*.join() + + assert 3 == cnt.get() + } } http://git-wip-us.apache.org/repos/asf/groovy/blob/785d6970/src/test/org/codehaus/groovy/runtime/memoize/MemoizeAtMostTest.groovy ---------------------------------------------------------------------- diff --git a/src/test/org/codehaus/groovy/runtime/memoize/MemoizeAtMostTest.groovy b/src/test/org/codehaus/groovy/runtime/memoize/MemoizeAtMostTest.groovy index c8c38a4..23279c6 100644 --- a/src/test/org/codehaus/groovy/runtime/memoize/MemoizeAtMostTest.groovy +++ b/src/test/org/codehaus/groovy/runtime/memoize/MemoizeAtMostTest.groovy @@ -18,6 +18,8 @@ */ package org.codehaus.groovy.runtime.memoize +import java.util.concurrent.atomic.AtomicInteger + /** * @author Vaclav Pech */ @@ -69,4 +71,16 @@ public class MemoizeAtMostTest extends AbstractMemoizeTestCase { assert 10 == mem(5) assert flag } + + public void testMemoizeAtMostConcurrently() { + AtomicInteger cnt = new AtomicInteger(0) + Closure cl = { + cnt.incrementAndGet() + it * 2 + } + Closure mem = cl.memoizeAtMost(3) + [4, 5, 6, 4, 5, 6, 4, 5, 6, 4, 5, 6].collect { num -> Thread.start { mem(num) } }*.join() + + assert 3 == cnt.get() + } } http://git-wip-us.apache.org/repos/asf/groovy/blob/785d6970/src/test/org/codehaus/groovy/runtime/memoize/MemoizeBetweenTest.groovy ---------------------------------------------------------------------- diff --git a/src/test/org/codehaus/groovy/runtime/memoize/MemoizeBetweenTest.groovy b/src/test/org/codehaus/groovy/runtime/memoize/MemoizeBetweenTest.groovy index 1ab472b..0f40c0d 100644 --- a/src/test/org/codehaus/groovy/runtime/memoize/MemoizeBetweenTest.groovy +++ b/src/test/org/codehaus/groovy/runtime/memoize/MemoizeBetweenTest.groovy @@ -18,6 +18,8 @@ */ package org.codehaus.groovy.runtime.memoize +import java.util.concurrent.atomic.AtomicInteger + /** * @author Vaclav Pech */ @@ -83,4 +85,16 @@ public class MemoizeBetweenTest extends AbstractMemoizeTestCase { assert 10 == mem(5) assert flag } + + public void testMemoizeBetweenConcurrently() { + AtomicInteger cnt = new AtomicInteger(0) + Closure cl = { + cnt.incrementAndGet() + it * 2 + } + Closure mem = cl.memoizeBetween(3, 3) + [4, 5, 6, 4, 5, 6, 4, 5, 6, 4, 5, 6].collect { num -> Thread.start { mem(num) } }*.join() + + assert 3 == cnt.get() + } }