svn commit: r1467579 - in /lucene/dev/trunk/lucene: ./ core/src/java/org/apache/lucene/util/ core/src/test/org/apache/lucene/util/

[us...@apache.org]

Author: uschindler
Date: Sat Apr 13 09:28:35 2013
New Revision: 1467579

URL: http://svn.apache.org/r1467579
Log:
LUCENE-4930: Reduce contention in older/buggy JVMs when using AttributeSource#addAttribute() because java.lang.ref.ReferenceQueue#poll() is implemented using synchronization.

Modified:
    lucene/dev/trunk/lucene/CHANGES.txt
    lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/AttributeSource.java
    lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/VirtualMethod.java
    lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/WeakIdentityMap.java
    lucene/dev/trunk/lucene/core/src/test/org/apache/lucene/util/TestWeakIdentityMap.java

Modified: lucene/dev/trunk/lucene/CHANGES.txt
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/CHANGES.txt?rev=1467579&r1=1467578&r2=1467579&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/CHANGES.txt (original)
+++ lucene/dev/trunk/lucene/CHANGES.txt Sat Apr 13 09:28:35 2013
@@ -216,6 +216,11 @@ Optimizations
 
 * LUCENE-4926: Speed up DisjunctionMatchQuery.  (Robert Muir)
 
+* LUCENE-4930: Reduce contention in older/buggy JVMs when using
+  AttributeSource#addAttribute() because java.lang.ref.ReferenceQueue#poll()
+  is implemented using synchronization.  (Christian Ziech, Karl Wright,
+  Uwe Schindler)
+
 API Changes
 
 * LUCENE-4844: removed TaxonomyReader.getParent(), you should use

Modified: lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/AttributeSource.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/AttributeSource.java?rev=1467579&r1=1467578&r2=1467579&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/AttributeSource.java (original)
+++ lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/AttributeSource.java Sat Apr 13 09:28:35 2013
@@ -55,9 +55,9 @@ public class AttributeSource {
     
     private static final class DefaultAttributeFactory extends AttributeFactory {
       private static final WeakIdentityMap<Class<? extends Attribute>, WeakReference<Class<? extends AttributeImpl>>> attClassImplMap =
-        WeakIdentityMap.newConcurrentHashMap();
+        WeakIdentityMap.newConcurrentHashMap(false);
       
-      private DefaultAttributeFactory() {}
+      DefaultAttributeFactory() {}
     
       @Override
       public AttributeImpl createAttributeInstance(Class<? extends Attribute> attClass) {
@@ -201,7 +201,7 @@ public class AttributeSource {
   
   /** a cache that stores all interfaces for known implementation classes for performance (slow reflection) */
   private static final WeakIdentityMap<Class<? extends AttributeImpl>,LinkedList<WeakReference<Class<? extends Attribute>>>> knownImplClasses =
-    WeakIdentityMap.newConcurrentHashMap();
+    WeakIdentityMap.newConcurrentHashMap(false);
   
   static LinkedList<WeakReference<Class<? extends Attribute>>> getAttributeInterfaces(final Class<? extends AttributeImpl> clazz) {
     LinkedList<WeakReference<Class<? extends Attribute>>> foundInterfaces = knownImplClasses.get(clazz);

Modified: lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/VirtualMethod.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/VirtualMethod.java?rev=1467579&r1=1467578&r2=1467579&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/VirtualMethod.java (original)
+++ lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/VirtualMethod.java Sat Apr 13 09:28:35 2013
@@ -63,7 +63,7 @@ public final class VirtualMethod<C> {
   private final Class<C> baseClass;
   private final String method;
   private final Class<?>[] parameters;
-  private final WeakIdentityMap<Class<? extends C>, Integer> cache = WeakIdentityMap.newConcurrentHashMap();
+  private final WeakIdentityMap<Class<? extends C>, Integer> cache = WeakIdentityMap.newConcurrentHashMap(false);
 
   /**
    * Creates a new instance for the given {@code baseClass} and method declaration.

Modified: lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/WeakIdentityMap.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/WeakIdentityMap.java?rev=1467579&r1=1467578&r2=1467579&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/WeakIdentityMap.java (original)
+++ lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/WeakIdentityMap.java Sat Apr 13 09:28:35 2013
@@ -44,25 +44,65 @@ import java.util.concurrent.ConcurrentHa
  * if not implemented carefully. The map only contains {@link Iterator} implementations
  * on the values and not-GCed keys. Lucene's implementation also supports {@code null}
  * keys, but those are never weak!
+ * 
+ * <p><a name="reapInfo" />The map supports two modes of operation:
+ * <ul>
+ *  <li>{@code reapOnRead = true}: This behaves identical to a {@link java.util.WeakHashMap}
+ *  where it also cleans up the reference queue on every read operation ({@link #get(Object)},
+ *  {@link #containsKey(Object)}, {@link #size()}, {@link #valueIterator()}), freeing map entries
+ *  of already GCed keys.</li>
+ *  <li>{@code reapOnRead = false}: This mode does not call {@link #reap()} on every read
+ *  operation. In this case, the reference queue is only cleaned up on write operations
+ *  (like {@link #put(Object, Object)}). This is ideal for maps with few entries where
+ *  the keys are unlikely be garbage collected, but there are lots of {@link #get(Object)}
+ *  operations. The code can still call {@link #reap()} to manually clean up the queue without
+ *  doing a write operation.</li>
+ * </ul>
  *
  * @lucene.internal
  */
 public final class WeakIdentityMap<K,V> {
   private final ReferenceQueue<Object> queue = new ReferenceQueue<Object>();
   private final Map<IdentityWeakReference, V> backingStore;
+  private final boolean reapOnRead;
 
-  /** 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 non-synchronized {@link HashMap}.
+   * The map <a href="#reapInfo">cleans up the reference queue on every read operation</a>.
+   */
+  public static <K,V> WeakIdentityMap<K,V> newHashMap() {
+    return newHashMap(true);
+  }
+
+  /**
+   * Creates a new {@code WeakIdentityMap} based on a non-synchronized {@link HashMap}.
+   * @param reapOnRead controls if the map <a href="#reapInfo">cleans up the reference queue on every read operation</a>.
+   */
+  public static <K,V> WeakIdentityMap<K,V> newHashMap(boolean reapOnRead) {
+    return new WeakIdentityMap<K,V>(new HashMap<IdentityWeakReference,V>(), reapOnRead);
+  }
+
+  /**
+   * Creates a new {@code WeakIdentityMap} based on a {@link ConcurrentHashMap}.
+   * The map <a href="#reapInfo">cleans up the reference queue on every read operation</a>.
+   */
+  public static <K,V> WeakIdentityMap<K,V> newConcurrentHashMap() {
+    return newConcurrentHashMap(true);
   }
 
-  /** 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>());
+  /**
+   * Creates a new {@code WeakIdentityMap} based on a {@link ConcurrentHashMap}.
+   * @param reapOnRead controls if the map <a href="#reapInfo">cleans up the reference queue on every read operation</a>.
+   */
+  public static <K,V> WeakIdentityMap<K,V> newConcurrentHashMap(boolean reapOnRead) {
+    return new WeakIdentityMap<K,V>(new ConcurrentHashMap<IdentityWeakReference,V>(), reapOnRead);
   }
 
-  private WeakIdentityMap(Map<IdentityWeakReference, V> backingStore) {
+  /** Private only constructor, to create use the static factory methods. */
+  private WeakIdentityMap(Map<IdentityWeakReference, V> backingStore, boolean reapOnRead) {
     this.backingStore = backingStore;
+    this.reapOnRead = reapOnRead;
   }
 
   /** Removes all of the mappings from this map. */
@@ -73,13 +113,13 @@ public final class WeakIdentityMap<K,V> 
 
   /** Returns {@code true} if this map contains a mapping for the specified key. */
   public boolean containsKey(Object key) {
-    reap();
+    if (reapOnRead) reap();
     return backingStore.containsKey(new IdentityWeakReference(key, null));
   }
 
   /** Returns the value to which the specified key is mapped. */
   public V get(Object key) {
-    reap();
+    if (reapOnRead) reap();
     return backingStore.get(new IdentityWeakReference(key, null));
   }
 
@@ -113,7 +153,7 @@ public final class WeakIdentityMap<K,V> 
   public int size() {
     if (backingStore.isEmpty())
       return 0;
-    reap();
+    if (reapOnRead) reap();
     return backingStore.size();
   }
   
@@ -178,13 +218,21 @@ public final class WeakIdentityMap<K,V> 
   
   /** Returns an iterator over all values of this map.
    * This iterator may return values whose key is already
-   * garbage collected while iterator is consumed. */
+   * garbage collected while iterator is consumed,
+   * especially if {@code reapOnRead} is {@code false}. */
   public Iterator<V> valueIterator() {
-    reap();
+    if (reapOnRead) reap();
     return backingStore.values().iterator();
   }
 
-  private void reap() {
+  /**
+   * This method manually cleans up the reference queue to remove all garbage
+   * collected key/value pairs from the map. Calling this method is not needed
+   * if {@code reapOnRead = true}. Otherwise it might be a good idea
+   * to call this method when there is spare time (e.g. from a background thread).
+   * @see <a href="#reapInfo">Information about the <code>reapOnRead</code> setting</a>
+   */
+  public void reap() {
     Reference<?> zombie;
     while ((zombie = queue.poll()) != null) {
       backingStore.remove(zombie);

Modified: lucene/dev/trunk/lucene/core/src/test/org/apache/lucene/util/TestWeakIdentityMap.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/core/src/test/org/apache/lucene/util/TestWeakIdentityMap.java?rev=1467579&r1=1467578&r2=1467579&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/core/src/test/org/apache/lucene/util/TestWeakIdentityMap.java (original)
+++ lucene/dev/trunk/lucene/core/src/test/org/apache/lucene/util/TestWeakIdentityMap.java Sat Apr 13 09:28:35 2013
@@ -18,7 +18,6 @@
 package org.apache.lucene.util;
 
 import java.util.Iterator;
-import java.util.Map;
 import java.util.NoSuchElementException;
 import java.util.Random;
 import java.util.concurrent.atomic.AtomicReferenceArray;
@@ -30,7 +29,7 @@ public class TestWeakIdentityMap extends
 
   public void testSimpleHashMap() {
     final WeakIdentityMap<String,String> map =
-      WeakIdentityMap.newHashMap();
+      WeakIdentityMap.newHashMap(random().nextBoolean());
     // we keep strong references to the keys,
     // so WeakIdentityMap will not forget about them:
     String key1 = new String("foo");
@@ -165,7 +164,7 @@ public class TestWeakIdentityMap extends
     final int threadCount = 8, keyCount = 1024;
     final ExecutorService exec = Executors.newFixedThreadPool(threadCount, new NamedThreadFactory("testConcurrentHashMap"));
     final WeakIdentityMap<Object,Integer> map =
-      WeakIdentityMap.newConcurrentHashMap();
+      WeakIdentityMap.newConcurrentHashMap(random().nextBoolean());
     // we keep strong references to the keys,
     // so WeakIdentityMap will not forget about them:
     final AtomicReferenceArray<Object> keys = new AtomicReferenceArray<Object>(keyCount);