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

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<K extends Serializable, V extends Object>
-    implements SimpleCache<K, V>, BeanNameAware
+    implements SimpleCache<K, V>, BeanNameAware, InitializingBean
 {
-    private final Map<K, V> map;
-    private int maxItems = 0;
+    private Map<K, AbstractMap.SimpleImmutableEntry<K, V>> 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<K, V>) Collections.synchronizedMap(new LinkedHashMap<K, V>(16, 0.75f, true) {
-            private static final long serialVersionUID = 1L;
-            @Override
-            protected boolean removeEldestEntry(Entry<K, V> eldest)
-            {
-                return maxItems > 0 && size() > maxItems;
-            }
-        });
     }
     
     @Override
@@ -69,13 +65,19 @@ public final class DefaultSimpleCache<K extends Serializable, V extends Object>
     @Override
     public V get(K key)
     {
-        return map.get(key);
+        AbstractMap.SimpleImmutableEntry<K, V> 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<K, V> kvp = new AbstractMap.SimpleImmutableEntry<K, V>(key, value);
+        map.put(key, kvp);
     }
 
     @Override
@@ -97,20 +99,15 @@ public final class DefaultSimpleCache<K extends Serializable, V extends Object>
     }
 
     /**
-     * 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;
     }
-    }
 
     /**
      * Since there are many cache instances, it is useful to be able to associate
@@ -123,4 +120,26 @@ public final class DefaultSimpleCache<K extends Serializable, V extends Object>
     {
         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<K, AbstractMap.SimpleImmutableEntry<K, V>>()
+                    .maximumWeightedCapacity(maxItems)
+                    .concurrencyLevel(32)
+                    .weigher(Weighers.singleton())
+                    .build();
+    }
 }

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<Integer, String> cache;
     
     @Before
-    public void setUp()
+    public void setUp() throws Exception
     {
         cache = new DefaultSimpleCache<Integer, String>();
+        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<Integer> it = cache.getKeys().iterator();
+        List<Integer> keys = new ArrayList<Integer>(cache.getKeys());
+        Collections.sort(keys);
+        
+        Iterator<Integer> 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");
 
-        cache.setMaxItems(10);
+        Iterator<Integer> i = cache.getKeys().iterator();
+        i.next();
+        i.next();
         
-        // The item should have gone.
-        assertFalse(cache.contains(1));
+        cache.put(5, "5");
+        
+        // Causes a ConcurrentModificationException with a java.util.LinkedHashMap
+        i.next();
     }
 }