LUCENE-3653: Improve the sophisticated™ backwards layers (VirtualMethod) and instantiation cost in AttributeSource/AttributeFactory (new reflection cache using ConcurrentHashMap has lockless get)

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1220458 13f79535-47bb-0310-9956-ffa450edef68

lucene/CHANGES.txt

@@ -766,6 +766,12 @@ Bug fixes
   double precision and to compute age to be how long ago the searcher
   was replaced with a new searcher (Mike McCandless)
 
+Optimizations
+
+* LUCENE-3653: Improve concurrency in VirtualMethod and AttributeSource by
+  using a WeakIdentityMap based on a ConcurrentHashMap.  (Uwe Schindler,
+  Gerrit Jansen van Vuuren)
+
 Documentation
 
 * LUCENE-3597: Fixed incorrect grouping documentation. (Martijn van Groningen, Robert Muir)

lucene/NOTICE.txt

@@ -30,6 +30,9 @@ with the same name. The implementation part is mainly done using pre-existing
 Lucene sorting code. In-place stable mergesort was borrowed from CGLIB,
 which is Apache-licensed.
 
+The class org.apache.lucene.util.WeakIdentityMap was derived from
+the Apache CXF project and is Apache License 2.0.
+
 The Google Code Prettify is Apache License 2.0.
 See http://code.google.com/p/google-code-prettify/
 

lucene/src/java/org/apache/lucene/util/AttributeSource.java

@@ -22,7 +22,6 @@ import java.util.Collections;
 import java.util.NoSuchElementException;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
-import java.util.WeakHashMap;
 import java.util.LinkedList;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -55,8 +54,8 @@ public class AttributeSource {
     public static final AttributeFactory DEFAULT_ATTRIBUTE_FACTORY = new DefaultAttributeFactory();
     
     private static final class DefaultAttributeFactory extends AttributeFactory {
-      private static final WeakHashMap<Class<? extends Attribute>, WeakReference<Class<? extends AttributeImpl>>> attClassImplMap =
-        new WeakHashMap<Class<? extends Attribute>, WeakReference<Class<? extends AttributeImpl>>>();
+      private static final WeakIdentityMap<Class<? extends Attribute>, WeakReference<Class<? extends AttributeImpl>>> attClassImplMap =
+        WeakIdentityMap.newConcurrentHashMap();
       
       private DefaultAttributeFactory() {}
     
@@ -72,23 +71,22 @@ public class AttributeSource {
       }
       
       private static Class<? extends AttributeImpl> getClassForInterface(Class<? extends Attribute> attClass) {
-        synchronized(attClassImplMap) {
-          final WeakReference<Class<? extends AttributeImpl>> ref = attClassImplMap.get(attClass);
-          Class<? extends AttributeImpl> clazz = (ref == null) ? null : ref.get();
-          if (clazz == null) {
-            try {
-              attClassImplMap.put(attClass,
-                new WeakReference<Class<? extends AttributeImpl>>(
-                  clazz = Class.forName(attClass.getName() + "Impl", true, attClass.getClassLoader())
-                  .asSubclass(AttributeImpl.class)
-                )
-              );
-            } catch (ClassNotFoundException e) {
-              throw new IllegalArgumentException("Could not find implementing class for " + attClass.getName());
-            }
+        final WeakReference<Class<? extends AttributeImpl>> ref = attClassImplMap.get(attClass);
+        Class<? extends AttributeImpl> clazz = (ref == null) ? null : ref.get();
+        if (clazz == null) {
+          // we have the slight chance that another thread may do the same, but who cares?
+          try {
+            attClassImplMap.put(attClass,
+              new WeakReference<Class<? extends AttributeImpl>>(
+                clazz = Class.forName(attClass.getName() + "Impl", true, attClass.getClassLoader())
+                .asSubclass(AttributeImpl.class)
+              )
+            );
+          } catch (ClassNotFoundException e) {
+            throw new IllegalArgumentException("Could not find implementing class for " + attClass.getName());
           }
-          return clazz;
         }
+        return clazz;
       }
     }
   }
@@ -199,30 +197,28 @@ public class AttributeSource {
   }
   
   /** a cache that stores all interfaces for known implementation classes for performance (slow reflection) */
-  private static final WeakHashMap<Class<? extends AttributeImpl>,LinkedList<WeakReference<Class<? extends Attribute>>>> knownImplClasses =
-    new WeakHashMap<Class<? extends AttributeImpl>,LinkedList<WeakReference<Class<? extends Attribute>>>>();
+  private static final WeakIdentityMap<Class<? extends AttributeImpl>,LinkedList<WeakReference<Class<? extends Attribute>>>> knownImplClasses =
+    WeakIdentityMap.newConcurrentHashMap();
   
   static LinkedList<WeakReference<Class<? extends Attribute>>> getAttributeInterfaces(final Class<? extends AttributeImpl> clazz) {
-    synchronized(knownImplClasses) {
-      LinkedList<WeakReference<Class<? extends Attribute>>> foundInterfaces = knownImplClasses.get(clazz);
-      if (foundInterfaces == null) {
-        // we have a strong reference to the class instance holding all interfaces in the list (parameter "att"),
-        // so all WeakReferences are never evicted by GC
-        knownImplClasses.put(clazz, foundInterfaces = new LinkedList<WeakReference<Class<? extends Attribute>>>());
-        // find all interfaces that this attribute instance implements
-        // and that extend the Attribute interface
-        Class<?> actClazz = clazz;
-        do {
-          for (Class<?> curInterface : actClazz.getInterfaces()) {
-            if (curInterface != Attribute.class && Attribute.class.isAssignableFrom(curInterface)) {
-              foundInterfaces.add(new WeakReference<Class<? extends Attribute>>(curInterface.asSubclass(Attribute.class)));
-            }
+    LinkedList<WeakReference<Class<? extends Attribute>>> foundInterfaces = knownImplClasses.get(clazz);
+    if (foundInterfaces == null) {
+      // we have the slight chance that another thread may do the same, but who cares?
+      foundInterfaces = new LinkedList<WeakReference<Class<? extends Attribute>>>();
+      // find all interfaces that this attribute instance implements
+      // and that extend the Attribute interface
+      Class<?> actClazz = clazz;
+      do {
+        for (Class<?> curInterface : actClazz.getInterfaces()) {
+          if (curInterface != Attribute.class && Attribute.class.isAssignableFrom(curInterface)) {
+            foundInterfaces.add(new WeakReference<Class<? extends Attribute>>(curInterface.asSubclass(Attribute.class)));
           }
-          actClazz = actClazz.getSuperclass();
-        } while (actClazz != null);
-      }
-      return foundInterfaces;
+        }
+        actClazz = actClazz.getSuperclass();
+      } while (actClazz != null);
+      knownImplClasses.put(clazz, foundInterfaces);
     }
+    return foundInterfaces;
   }
   
   /** <b>Expert:</b> Adds a custom AttributeImpl instance with one or more Attribute interfaces.

lucene/src/java/org/apache/lucene/util/VirtualMethod.java

@@ -20,7 +20,6 @@ package org.apache.lucene.util;
 import java.lang.reflect.Method;
 import java.util.Collections;
 import java.util.HashSet;
-import java.util.WeakHashMap;
 import java.util.Set;
 
 /**
@@ -64,8 +63,7 @@ public final class VirtualMethod<C> {
   private final Class<C> baseClass;
   private final String method;
   private final Class<?>[] parameters;
-  private final WeakHashMap<Class<? extends C>, Integer> cache =
-    new WeakHashMap<Class<? extends C>, Integer>();
+  private final WeakIdentityMap<Class<? extends C>, Integer> cache = WeakIdentityMap.newConcurrentHashMap();
 
   /**
    * Creates a new instance for the given {@code baseClass} and method declaration.
@@ -93,9 +91,10 @@ public final class VirtualMethod<C> {
    * in the inheritance path between {@code baseClass} and the given subclass {@code subclazz}.
    * @return 0 iff not overridden, else the distance to the base class
    */
-  public synchronized int getImplementationDistance(final Class<? extends C> subclazz) {
+  public int getImplementationDistance(final Class<? extends C> subclazz) {
     Integer distance = cache.get(subclazz);
     if (distance == null) {
+      // we have the slight chance that another thread may do the same, but who cares?
       cache.put(subclazz, distance = Integer.valueOf(reflectImplementationDistance(subclazz)));
     }
     return distance.intValue();

lucene/src/java/org/apache/lucene/util/WeakIdentityMap.java

@@ -0,0 +1,137 @@
+package org.apache.lucene.util;
+
+/**
+ * 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.
+ */
+
+import java.lang.ref.Reference;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Implements a combination of {@link java.util.WeakHashMap} and
+ * {@link java.util.IdentityHashMap}.
+ * Useful for caches that need to key off of a {@code ==} comparison
+ * instead of a {@code .equals}.
+ * 
+ * <p>This class is not a general-purpose {@link java.util.Map}
+ * implementation! It intentionally violates
+ * Map's general contract, which mandates the use of the equals method
+ * when comparing objects. This class is designed for use only in the
+ * rare cases wherein reference-equality semantics are required.
+ * 
+ * <p>This implementation was forked from <a href="http://cxf.apache.org/">Apache CXF</a>
+ * but modified to <b>not</b> implement the {@link java.util.Map} interface and
+ * without any set/iterator views on it, as those are error-prone
+ * and inefficient, if not implemented carefully. Lucene's implementation also
+ * supports {@code null} keys, but those are never weak!
+ *
+ * @lucene.internal
+ */
+public final class WeakIdentityMap<K,V> {
+  private final ReferenceQueue<Object> queue = new ReferenceQueue<Object>();
+  private final Map<IdentityWeakReference, V> backingStore;
+
+  /** Creates a new {@code WeakIdentityMap} based on a non-synchronized {@link HashMap}. */
+  public static final <K,V> WeakIdentityMap<K,V> newHashMap() {
+    return new WeakIdentityMap<K,V>(new HashMap<IdentityWeakReference,V>());
+  }
+
+  /** Creates a new {@code WeakIdentityMap} based on a {@link ConcurrentHashMap}. */
+  public static final <K,V> WeakIdentityMap<K,V> newConcurrentHashMap() {
+    return new WeakIdentityMap<K,V>(new ConcurrentHashMap<IdentityWeakReference,V>());
+  }
+
+  private WeakIdentityMap(Map<IdentityWeakReference, V> backingStore) {
+    this.backingStore = backingStore;
+  }
+
+  public void clear() {
+    backingStore.clear();
+    reap();
+  }
+
+  public boolean containsKey(Object key) {
+    reap();
+    return backingStore.containsKey(new IdentityWeakReference(key, queue));
+  }
+
+  public V get(Object key) {
+    reap();
+    return backingStore.get(new IdentityWeakReference(key, queue));
+  }
+
+  public V put(K key, V value) {
+    reap();
+    return backingStore.put(new IdentityWeakReference(key, queue), value);
+  }
+
+  public boolean isEmpty() {
+    return size() == 0;
+  }
+
+  public V remove(Object key) {
+    reap();
+    return backingStore.remove(new IdentityWeakReference(key, queue));
+  }
+
+  public int size() {
+    if (backingStore.isEmpty())
+      return 0;
+    reap();
+    return backingStore.size();
+  }
+
+  private void reap() {
+    Reference<?> zombie;
+    while ((zombie = queue.poll()) != null) {
+      backingStore.remove(zombie);
+    }
+  }
+
+  private static final class IdentityWeakReference extends WeakReference<Object> {
+    private final int hash;
+    
+    IdentityWeakReference(Object obj, ReferenceQueue<Object> queue) {
+      super(obj == null ? NULL : obj, queue);
+      hash = System.identityHashCode(obj);
+    }
+
+    public int hashCode() {
+      return hash;
+    }
+
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
+      if (o instanceof IdentityWeakReference) {
+        final IdentityWeakReference ref = (IdentityWeakReference)o;
+        if (this.get() == ref.get()) {
+          return true;
+        }
+      }
+      return false;
+    }
+  
+    // we keep a hard reference to our NULL key, so map supports null keys that never get GCed:
+    private static final Object NULL = new Object();
+  }
+}
+

lucene/src/test/org/apache/lucene/util/TestWeakIdentityMap.java

@@ -0,0 +1,193 @@
+/**
+ * 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.apache.lucene.util;
+
+import java.util.Map;
+import java.util.Random;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReferenceArray;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.TimeUnit;
+
+public class TestWeakIdentityMap extends LuceneTestCase {
+
+  public void testSimpleHashMap() {
+    final WeakIdentityMap<String,String> map =
+      WeakIdentityMap.newHashMap();
+    // we keep strong references to the keys,
+    // so WeakIdentityMap will not forget about them:
+    String key1 = new String("foo");
+    String key2 = new String("foo");
+    String key3 = new String("foo");
+    
+    assertNotSame(key1, key2);
+    assertEquals(key1, key2);
+    assertNotSame(key1, key3);
+    assertEquals(key1, key3);
+    assertNotSame(key2, key3);
+    assertEquals(key2, key3);
+
+    map.put(key1, "bar1");
+    map.put(key2, "bar2");
+    map.put(null, "null");
+    
+    assertEquals(3, map.size());
+
+    assertEquals("bar1", map.get(key1));
+    assertEquals("bar2", map.get(key2));
+    assertEquals(null, map.get(key3));
+    assertEquals("null", map.get(null));
+    
+    assertTrue(map.containsKey(key1));
+    assertTrue(map.containsKey(key2));
+    assertFalse(map.containsKey(key3));
+    assertTrue(map.containsKey(null));
+
+    // repeat and check that we have no double entries
+    map.put(key1, "bar1");
+    map.put(key2, "bar2");
+    map.put(null, "null");
+
+    assertEquals(3, map.size());
+    
+    assertEquals("bar1", map.get(key1));
+    assertEquals("bar2", map.get(key2));
+    assertEquals(null, map.get(key3));
+    assertEquals("null", map.get(null));
+    
+    assertTrue(map.containsKey(key1));
+    assertTrue(map.containsKey(key2));
+    assertFalse(map.containsKey(key3));
+    assertTrue(map.containsKey(null));
+
+    map.remove(null);
+    assertEquals(2, map.size());
+    map.remove(key1);
+    assertEquals(1, map.size());
+    map.put(key1, "bar1");
+    map.put(key2, "bar2");
+    map.put(key3, "bar3");
+    assertEquals(3, map.size());
+    
+    // clear strong refs
+    key1 = key2 = key3 = null;
+    
+    // check that GC does not cause problems in reap() method, wait 1 second and let GC work:
+    int size = map.size();
+    for (int i = 0; size > 0 && i < 10; i++) try {
+      System.runFinalization();
+      System.gc();
+      Thread.sleep(100L);
+      assertTrue(size >= map.size());
+      size = map.size();
+    } catch (InterruptedException ie) {}
+
+    map.clear();
+    assertEquals(0, map.size());
+    assertTrue(map.isEmpty());
+    
+    key1 = new String("foo");
+    key2 = new String("foo");
+    map.put(key1, "bar1");
+    map.put(key2, "bar2");
+    assertEquals(2, map.size());
+    
+    map.clear();
+    assertEquals(0, map.size());
+    assertTrue(map.isEmpty());
+  }
+
+  public void testConcurrentHashMap() throws Exception {
+    final int threadCount = atLeast(32), keyCount = atLeast(1024);
+    final ExecutorService exec = Executors.newFixedThreadPool(threadCount + 1);
+    final WeakIdentityMap<Object,Integer> map =
+      WeakIdentityMap.newConcurrentHashMap();
+    // we keep strong references to the keys,
+    // so WeakIdentityMap will not forget about them:
+    final AtomicReferenceArray<Object> keys = new AtomicReferenceArray<Object>(keyCount);
+    for (int j = 0; j < keyCount; j++) {
+      keys.set(j, new Object());
+    }
+    
+    try {
+      final AtomicInteger running = new AtomicInteger(threadCount);
+      for (int t = 0; t < threadCount; t++) {
+        final Random rnd = new Random(random.nextLong());
+        final int count = atLeast(rnd, 20000);
+        exec.execute(new Runnable() {
+          public void run() {
+            for (int i = 0; i < count; i++) {
+              final int j = rnd.nextInt(keyCount);
+              switch (rnd.nextInt(4)) {
+                case 0:
+                  map.put(keys.get(j), Integer.valueOf(j));
+                  break;
+                case 1:
+                  final Integer v = map.get(keys.get(j));
+                  if (v != null) {
+                    assertEquals(j, v.intValue());
+                  }
+                  break;
+                case 2:
+                  map.remove(keys.get(j));
+                  break;
+                case 3:
+                  // renew key, the old one will be GCed at some time:
+                  keys.set(j, new Object());
+                  break;
+                default:
+                  fail("Should not get here.");
+              }
+            }
+            running.decrementAndGet();
+          }
+        });
+      }
+      exec.execute(new Runnable() {
+        public void run() {
+          // check that GC does not cause problems in reap() method:
+          while (running.get() > 0) {
+            System.runFinalization();
+            System.gc();
+            map.isEmpty(); // simple access
+          }
+        }
+      });
+    } finally {
+      exec.shutdown();
+      while (!exec.awaitTermination(1000L, TimeUnit.MILLISECONDS));
+    }
+    
+    // clear strong refs
+    for (int j = 0; j < keyCount; j++) {
+      keys.set(j, null);
+    }
+    
+    // check that GC does not cause problems in reap() method:
+    int size = map.size();
+    for (int i = 0; size > 0 && i < 10; i++) try {
+      System.runFinalization();
+      System.gc();
+      Thread.sleep(100L);
+      assertTrue(size >= map.size());
+      size = map.size();
+    } catch (InterruptedException ie) {}
+  }
+
+}

solr/NOTICE.txt

@@ -69,6 +69,9 @@ with the same name. The implementation part is mainly done using pre-existing
 Lucene sorting code. In-place stable mergesort was borrowed from CGLIB,
 which is Apache-licensed.
 
+The class org.apache.lucene.util.WeakIdentityMap was derived from
+the Apache CXF project and is Apache License 2.0.
+
 The Google Code Prettify is Apache License 2.0.
 See http://code.google.com/p/google-code-prettify/