From 3ccd4f4e033d61e33c635593136c1c85a3ea08c6 Mon Sep 17 00:00:00 2001 From: Uwe Schindler Date: Sun, 18 Dec 2011 17:11:06 +0000 Subject: [PATCH] =?UTF-8?q?LUCENE-3653:=20=20Improve=20the=20sophisticated?= =?UTF-8?q?=E2=84=A2=20backwards=20layers=20(VirtualMethod)=20and=20instan?= =?UTF-8?q?tiation=20cost=20in=20AttributeSource/AttributeFactory=20(new?= =?UTF-8?q?=20reflection=20cache=20using=20ConcurrentHashMap=20has=20lockl?= =?UTF-8?q?ess=20get)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1220458 13f79535-47bb-0310-9956-ffa450edef68 --- lucene/CHANGES.txt | 6 + lucene/NOTICE.txt | 3 + .../apache/lucene/util/AttributeSource.java | 72 +++---- .../org/apache/lucene/util/VirtualMethod.java | 7 +- .../apache/lucene/util/WeakIdentityMap.java | 137 +++++++++++++ .../lucene/util/TestWeakIdentityMap.java | 193 ++++++++++++++++++ solr/NOTICE.txt | 3 + 7 files changed, 379 insertions(+), 42 deletions(-) create mode 100644 lucene/src/java/org/apache/lucene/util/WeakIdentityMap.java create mode 100644 lucene/src/test/org/apache/lucene/util/TestWeakIdentityMap.java diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index fa2b792854b..1208110bf88 100644 --- a/lucene/CHANGES.txt +++ b/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) diff --git a/lucene/NOTICE.txt b/lucene/NOTICE.txt index 35901b70c28..c26b0e2ba62 100644 --- a/lucene/NOTICE.txt +++ b/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/ diff --git a/lucene/src/java/org/apache/lucene/util/AttributeSource.java b/lucene/src/java/org/apache/lucene/util/AttributeSource.java index 631de15f3a5..e974df58ee9 100644 --- a/lucene/src/java/org/apache/lucene/util/AttributeSource.java +++ b/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, WeakReference>> attClassImplMap = - new WeakHashMap, WeakReference>>(); + private static final WeakIdentityMap, WeakReference>> attClassImplMap = + WeakIdentityMap.newConcurrentHashMap(); private DefaultAttributeFactory() {} @@ -72,23 +71,22 @@ public class AttributeSource { } private static Class getClassForInterface(Class attClass) { - synchronized(attClassImplMap) { - final WeakReference> ref = attClassImplMap.get(attClass); - Class clazz = (ref == null) ? null : ref.get(); - if (clazz == null) { - try { - attClassImplMap.put(attClass, - new WeakReference>( - 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> ref = attClassImplMap.get(attClass); + Class 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>( + 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,LinkedList>>> knownImplClasses = - new WeakHashMap,LinkedList>>>(); + private static final WeakIdentityMap,LinkedList>>> knownImplClasses = + WeakIdentityMap.newConcurrentHashMap(); static LinkedList>> getAttributeInterfaces(final Class clazz) { - synchronized(knownImplClasses) { - LinkedList>> 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>>()); - // 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>(curInterface.asSubclass(Attribute.class))); - } + LinkedList>> 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>>(); + // 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>(curInterface.asSubclass(Attribute.class))); } - actClazz = actClazz.getSuperclass(); - } while (actClazz != null); - } - return foundInterfaces; + } + actClazz = actClazz.getSuperclass(); + } while (actClazz != null); + knownImplClasses.put(clazz, foundInterfaces); } + return foundInterfaces; } /** Expert: Adds a custom AttributeImpl instance with one or more Attribute interfaces. diff --git a/lucene/src/java/org/apache/lucene/util/VirtualMethod.java b/lucene/src/java/org/apache/lucene/util/VirtualMethod.java index 11937db0542..c5590a7aa2d 100644 --- a/lucene/src/java/org/apache/lucene/util/VirtualMethod.java +++ b/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 { private final Class baseClass; private final String method; private final Class[] parameters; - private final WeakHashMap, Integer> cache = - new WeakHashMap, Integer>(); + private final WeakIdentityMap, Integer> cache = WeakIdentityMap.newConcurrentHashMap(); /** * Creates a new instance for the given {@code baseClass} and method declaration. @@ -93,9 +91,10 @@ public final class VirtualMethod { * 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 subclazz) { + public int getImplementationDistance(final Class 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(); diff --git a/lucene/src/java/org/apache/lucene/util/WeakIdentityMap.java b/lucene/src/java/org/apache/lucene/util/WeakIdentityMap.java new file mode 100644 index 00000000000..047fb407c17 --- /dev/null +++ b/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}. + * + *

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. + * + *

This implementation was forked from Apache CXF + * but modified to not 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 { + private final ReferenceQueue queue = new ReferenceQueue(); + private final Map backingStore; + + /** Creates a new {@code WeakIdentityMap} based on a non-synchronized {@link HashMap}. */ + public static final WeakIdentityMap newHashMap() { + return new WeakIdentityMap(new HashMap()); + } + + /** Creates a new {@code WeakIdentityMap} based on a {@link ConcurrentHashMap}. */ + public static final WeakIdentityMap newConcurrentHashMap() { + return new WeakIdentityMap(new ConcurrentHashMap()); + } + + private WeakIdentityMap(Map 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 { + private final int hash; + + IdentityWeakReference(Object obj, ReferenceQueue 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(); + } +} + diff --git a/lucene/src/test/org/apache/lucene/util/TestWeakIdentityMap.java b/lucene/src/test/org/apache/lucene/util/TestWeakIdentityMap.java new file mode 100644 index 00000000000..cfbba3d8033 --- /dev/null +++ b/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 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 map = + WeakIdentityMap.newConcurrentHashMap(); + // we keep strong references to the keys, + // so WeakIdentityMap will not forget about them: + final AtomicReferenceArray keys = new AtomicReferenceArray(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) {} + } + +} diff --git a/solr/NOTICE.txt b/solr/NOTICE.txt index 3896176237a..140a11564d6 100644 --- a/solr/NOTICE.txt +++ b/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/