From 99b47ce435347e2c227354a2b3a5b6c13349dd90 Mon Sep 17 00:00:00 2001 From: Matt Ward Date: Mon, 1 Oct 2012 16:47:43 +0000 Subject: [PATCH] ALF-15888: change implementation of DefaultSimpleCache to use Google's ConcurrentLinkedHashMap. After much deliberation I decided not to offer an unbounded cache size, i.e. maxItems MUST be at least one. This simplifies the implementation (marginally) and means that tests do not have to be duplicated for both underlying data structure types (better coverage). Would we really want a cache to grow indefinitely? git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@42223 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261 --- .../repo/cache/DefaultSimpleCache.java | 75 ++++++++++++------- .../repo/cache/DefaultSimpleCacheTest.java | 47 ++++++------ 2 files changed, 70 insertions(+), 52 deletions(-) diff --git a/source/java/org/alfresco/repo/cache/DefaultSimpleCache.java b/source/java/org/alfresco/repo/cache/DefaultSimpleCache.java index 948d0ef641..68434810ac 100644 --- a/source/java/org/alfresco/repo/cache/DefaultSimpleCache.java +++ b/source/java/org/alfresco/repo/cache/DefaultSimpleCache.java @@ -19,39 +19,35 @@ package org.alfresco.repo.cache; import java.io.Serializable; +import java.util.AbstractMap; import java.util.Collection; -import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; -import java.util.Map.Entry; import org.springframework.beans.factory.BeanNameAware; +import org.springframework.beans.factory.InitializingBean; + +import com.googlecode.concurrentlinkedhashmap.ConcurrentLinkedHashMap; +import com.googlecode.concurrentlinkedhashmap.Weighers; /** - * {@link SimpleCache} implementation backed by a {@link LinkedHashMap}. + * {@link SimpleCache} implementation backed by a {@link ConcurrentLinkedHashMap}. * * @author Matt Ward */ public final class DefaultSimpleCache - implements SimpleCache, BeanNameAware + implements SimpleCache, BeanNameAware, InitializingBean { - private final Map map; - private int maxItems = 0; + private Map> map; + private int maxItems = 1000000; private String cacheName; + /** + * Default constructor. {@link #afterPropertiesSet()} MUST be called before the cache + * may be used when the cache is constructed using the default constructor. + */ public DefaultSimpleCache() { - // Create a LinkedHashMap with accessOrder true, i.e. iteration order - // will be least recently accessed first. Eviction policy will therefore be LRU. - // The map will have a bounded size determined by the maxItems member variable. - map = (Map) Collections.synchronizedMap(new LinkedHashMap(16, 0.75f, true) { - private static final long serialVersionUID = 1L; - @Override - protected boolean removeEldestEntry(Entry eldest) - { - return maxItems > 0 && size() > maxItems; - } - }); } @Override @@ -69,13 +65,19 @@ public final class DefaultSimpleCache @Override public V get(K key) { - return map.get(key); + AbstractMap.SimpleImmutableEntry kvp = map.get(key); + if (kvp == null) + { + return null; + } + return kvp.getValue(); } @Override public void put(K key, V value) { - map.put(key, value); + AbstractMap.SimpleImmutableEntry kvp = new AbstractMap.SimpleImmutableEntry(key, value); + map.put(key, kvp); } @Override @@ -97,19 +99,14 @@ public final class DefaultSimpleCache } /** - * Sets the maximum number of items that the cache will hold. Setting - * this value will cause the cache to be emptied. A value of zero - * will allow the cache to grow unbounded. + * Sets the maximum number of items that the cache will hold. The cache + * must be re-initialised if already in existence using {@link #afterPropertiesSet()}. * * @param maxItems */ - public void setMaxItems(int maxItems) + public synchronized void setMaxItems(int maxItems) { - synchronized(map) - { - map.clear(); - this.maxItems = maxItems; - } + this.maxItems = maxItems; } /** @@ -123,4 +120,26 @@ public final class DefaultSimpleCache { this.cacheName = cacheName; } + + + /** + * Initialise the cache. + * + * @throws Exception + */ + @Override + public synchronized void afterPropertiesSet() throws Exception + { + if (maxItems < 1) + { + throw new IllegalArgumentException("maxItems property must be a positive integer."); + } + + // The map will have a bounded size determined by the maxItems member variable. + map = new ConcurrentLinkedHashMap.Builder>() + .maximumWeightedCapacity(maxItems) + .concurrencyLevel(32) + .weigher(Weighers.singleton()) + .build(); + } } diff --git a/source/java/org/alfresco/repo/cache/DefaultSimpleCacheTest.java b/source/java/org/alfresco/repo/cache/DefaultSimpleCacheTest.java index 7f6dd7171e..0ad7d19462 100644 --- a/source/java/org/alfresco/repo/cache/DefaultSimpleCacheTest.java +++ b/source/java/org/alfresco/repo/cache/DefaultSimpleCacheTest.java @@ -23,7 +23,10 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import java.util.ArrayList; +import java.util.Collections; import java.util.Iterator; +import java.util.List; import org.junit.Before; import org.junit.Test; @@ -38,32 +41,19 @@ public class DefaultSimpleCacheTest private DefaultSimpleCache cache; @Before - public void setUp() + public void setUp() throws Exception { cache = new DefaultSimpleCache(); + cache.setMaxItems(100); + cache.afterPropertiesSet(); } @Test - public void unboundedSizeCache() - { - cache.put(1, "1"); - cache.put(2, "2"); - cache.put(3, "3"); - cache.put(4, "4"); - cache.put(5, "5"); - - assertEquals("1", cache.get(1)); - assertEquals("2", cache.get(2)); - assertEquals("3", cache.get(3)); - assertEquals("4", cache.get(4)); - assertEquals("5", cache.get(5)); - } - - @Test - public void boundedSizeCache() + public void boundedSizeCache() throws Exception { // We'll only keep the LAST 3 items cache.setMaxItems(3); + cache.afterPropertiesSet(); cache.put(1, "1"); cache.put(2, "2"); @@ -136,7 +126,10 @@ public class DefaultSimpleCacheTest cache.put(12, "red"); cache.put(43, "olive"); - Iterator it = cache.getKeys().iterator(); + List keys = new ArrayList(cache.getKeys()); + Collections.sort(keys); + + Iterator it = keys.iterator(); assertEquals(3, it.next().intValue()); assertEquals(12, it.next().intValue()); assertEquals(43, it.next().intValue()); @@ -144,14 +137,20 @@ public class DefaultSimpleCacheTest } @Test - public void clearUponSetMaxItems() + public void noConcurrentModificationException() { cache.put(1, "1"); - assertTrue(cache.contains(1)); + cache.put(2, "2"); + cache.put(3, "3"); + cache.put(4, "4"); + + Iterator i = cache.getKeys().iterator(); + i.next(); + i.next(); - cache.setMaxItems(10); + cache.put(5, "5"); - // The item should have gone. - assertFalse(cache.contains(1)); + // Causes a ConcurrentModificationException with a java.util.LinkedHashMap + i.next(); } }