From c2ead96cff06da8f4e433b23241217a6c131b6ed Mon Sep 17 00:00:00 2001 From: Gareth Davis Date: Mon, 16 Sep 2013 16:19:18 +0100 Subject: [PATCH] JAXB-979 ClassFactory constructor cache could be more efficient Replaces the ThreadLocal implementation with a shared WeakHashMap with a copy-on-write style update. --- .../java/com/sun/xml/bind/v2/ClassFactory.java | 144 ++++++++++++------ .../v2/runtime/unmarshaller/UnmarshallerImpl.java | 21 +-- .../java/com/sun/xml/bind/v2/ClassFactoryTest.java | 169 +++++++++++++++++++++ 3 files changed, 272 insertions(+), 62 deletions(-) create mode 100644 jaxb-ri/runtime/impl/src/test/java/com/sun/xml/bind/v2/ClassFactoryTest.java diff --git a/jaxb-ri/core/src/main/java/com/sun/xml/bind/v2/ClassFactory.java b/jaxb-ri/core/src/main/java/com/sun/xml/bind/v2/ClassFactory.java index 36641d7..94eb0df 100644 --- a/jaxb-ri/core/src/main/java/com/sun/xml/bind/v2/ClassFactory.java +++ b/jaxb-ri/core/src/main/java/com/sun/xml/bind/v2/ClassFactory.java @@ -47,6 +47,7 @@ import java.lang.reflect.Modifier; import java.lang.ref.WeakReference; import java.util.Map; import java.util.WeakHashMap; +import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Level; import java.util.logging.Logger; @@ -71,63 +72,41 @@ public final class ClassFactory { /** * Cache from a class to its default constructor. * - * To avoid synchronization among threads, we use {@link ThreadLocal}. + * Note that the {@code WeakHashMap) is */ - private static final ThreadLocal>> tls = new ThreadLocal>>() { - @Override - public Map> initialValue() { - return new WeakHashMap>(); - } - }; + private static final AtomicReference>> constructorCache = + new AtomicReference>>(new WeakHashMap>()); + + /** + * This method is a NOP. + * + *

Previously this method cleared the thread local cache state. This state has been removed and but the method + * remains as there will be client code out there that calls this method.

+ * + * @deprecated no longer valid + * @see #clear() + */ + @Deprecated public static void cleanCache() { - if (tls != null) { - try { - tls.remove(); - } catch (Exception e) { - logger.log(Level.WARNING, "Unable to clean Thread Local cache of classes used in Unmarshaller: {0}", e.getLocalizedMessage()); - } - } } - + + /** + * Completely clear any retrained cache information. + */ + public static void clear() { + constructorCache.set(new WeakHashMap>()); + } + /** * Creates a new instance of the class but throw exceptions without catching it. */ public static T create0( final Class clazz ) throws IllegalAccessException, InvocationTargetException, InstantiationException { - Map> m = tls.get(); - Constructor cons = null; - WeakReference consRef = m.get(clazz); - if(consRef!=null) - cons = consRef.get(); - if(cons==null) { - try { - cons = clazz.getDeclaredConstructor(emptyClass); - } catch (NoSuchMethodException e) { - logger.log(Level.INFO,"No default constructor found on "+clazz,e); - NoSuchMethodError exp; - if(clazz.getDeclaringClass()!=null && !Modifier.isStatic(clazz.getModifiers())) { - exp = new NoSuchMethodError(Messages.NO_DEFAULT_CONSTRUCTOR_IN_INNER_CLASS.format(clazz.getName())); - } else { - exp = new NoSuchMethodError(e.getMessage()); - } - exp.initCause(e); - throw exp; - } + Constructor cons = getCachedConstructor(clazz); - int classMod = clazz.getModifiers(); - - if(!Modifier.isPublic(classMod) || !Modifier.isPublic(cons.getModifiers())) { - // attempt to make it work even if the constructor is not accessible - try { - cons.setAccessible(true); - } catch(SecurityException e) { - // but if we don't have a permission to do so, work gracefully. - logger.log(Level.FINE,"Unable to make the constructor of "+clazz+" accessible",e); - throw e; - } - } - - m.put(clazz,new WeakReference(cons)); + if( cons == null ) { + cons = findAccessibleConstructorOrDie(clazz); + putConstructor(clazz, cons); } return cons.newInstance(emptyObject); @@ -222,4 +201,73 @@ public final class ClassFactory { // and returns null return null; } + + /** + * Update the constructor cache with the constructor for a the given class. + * + * @param clazz required class + * @param cons required constructor + * @param type of both + */ + private static void putConstructor(Class clazz, Constructor cons) { + + Map> currentMap, newMap; + WeakReference value = new WeakReference(cons); + + do { + currentMap = constructorCache.get(); + + newMap = new WeakHashMap>(currentMap); + newMap.put(clazz, value); + + } while( ! constructorCache.compareAndSet(currentMap, newMap) ); + } + + /** + * Attempt to get the constructor from the cache if possible. + * @param clazz required class to use as the cache key + * @param type of the constructor + * @return the cached constructor or null if not found + */ + @SuppressWarnings("unchecked") + private static Constructor getCachedConstructor(Class clazz) { + WeakReference ref = constructorCache.get().get(clazz); + + if( ref == null ) { + return null; // not found + } + + return ref.get(); // value may still be null as the reference is weak + } + + private static Constructor findAccessibleConstructorOrDie(Class clazz) { + Constructor cons; + try { + cons = clazz.getDeclaredConstructor(emptyClass); + } catch (NoSuchMethodException e) { + logger.log(Level.INFO,"No default constructor found on "+clazz,e); + NoSuchMethodError exp; + if(clazz.getDeclaringClass()!=null && !Modifier.isStatic(clazz.getModifiers())) { + exp = new NoSuchMethodError(Messages.NO_DEFAULT_CONSTRUCTOR_IN_INNER_CLASS.format(clazz.getName())); + } else { + exp = new NoSuchMethodError(e.getMessage()); + } + exp.initCause(e); + throw exp; + } + + int classMod = clazz.getModifiers(); + + if(!Modifier.isPublic(classMod) || !Modifier.isPublic(cons.getModifiers())) { + // attempt to make it work even if the constructor is not accessible + try { + cons.setAccessible(true); + } catch(SecurityException e) { + // but if we don't have a permission to do so, work gracefully. + logger.log(Level.FINE,"Unable to make the constructor of "+clazz+" accessible",e); + throw e; + } + } + return cons; + } } diff --git a/jaxb-ri/runtime/impl/src/main/java/com/sun/xml/bind/v2/runtime/unmarshaller/UnmarshallerImpl.java b/jaxb-ri/runtime/impl/src/main/java/com/sun/xml/bind/v2/runtime/unmarshaller/UnmarshallerImpl.java index d8e6f5c..dff41a6 100644 --- a/jaxb-ri/runtime/impl/src/main/java/com/sun/xml/bind/v2/runtime/unmarshaller/UnmarshallerImpl.java +++ b/jaxb-ri/runtime/impl/src/main/java/com/sun/xml/bind/v2/runtime/unmarshaller/UnmarshallerImpl.java @@ -613,22 +613,15 @@ import org.xml.sax.helpers.DefaultHandler; return coordinator; } - @Override - @SuppressWarnings("FinalizeDeclaration") - protected void finalize() throws Throwable { - try { - ClassFactory.cleanCache(); - } finally { - super.finalize(); - } - } - /** - * Must be called from same thread which created the UnmarshallerImpl instance. - * @throws IOException + * Currently a NOP. + * + *

Previous versions cleared the {@code ClassFactory} thread local cache but this is now as the + * ClassFactory doesn't have any {@code ThreadLocal} state.

*/ - public void close() throws IOException { - ClassFactory.cleanCache(); + public void close() { + // should really call clear() or something on JAXCContext as it actually retains a reference to the + // last unmarshalled object. } } diff --git a/jaxb-ri/runtime/impl/src/test/java/com/sun/xml/bind/v2/ClassFactoryTest.java b/jaxb-ri/runtime/impl/src/test/java/com/sun/xml/bind/v2/ClassFactoryTest.java new file mode 100644 index 0000000..c54fc2f --- /dev/null +++ b/jaxb-ri/runtime/impl/src/test/java/com/sun/xml/bind/v2/ClassFactoryTest.java @@ -0,0 +1,169 @@ +package com.sun.xml.bind.v2; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; + +import junit.framework.TestCase; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; + +/** + * + */ +public class ClassFactoryTest { + + @Test + public void testCreate_DefaultConstructor() { + DefaultConstructor result = ClassFactory.create(DefaultConstructor.class); + + assertNotNull(result); + } + + @Test + public void testCreate_NoVisibleConstructor() { + NoVisibleConstructor result = ClassFactory.create(NoVisibleConstructor.class); + assertNotNull( result ); + assertNotNull( result.getFoo() ); + } + + @Test + public void testCreate_PublicConstructor() { + PublicConstructor result = ClassFactory.create(PublicConstructor.class); + + assertNotNull( result ); + assertNotNull( result.getFoo() ); + } + + + @Test + public void testCreate_ManyThreads() throws InterruptedException, ExecutionException { + ExecutorService executorService = Executors.newFixedThreadPool(10); + try { + List> callables = generateCallablesFor(100, DefaultConstructor.class, + NoVisibleConstructor.class, PublicConstructor.class); + + List> results = executorService.invokeAll(callables); + + for( Future f : results) { + f.get(); // this will throw an Exception if the callable fails for some reason + } + } finally { + executorService.shutdownNow(); + } + } + + @Test + public void testClear_WithContention() throws InterruptedException, ExecutionException { + // tricky one to test.. since the effects of clear() are not visible outside of + // the ClassFactory. + ExecutorService executorService = Executors.newFixedThreadPool(10); + + try { + List> callables = new ArrayList>(201); + + callables.addAll(generateCallablesFor(100, DefaultConstructor.class, + NoVisibleConstructor.class, PublicConstructor.class)); + + callables.add(new Callable() { + public Object call() throws Exception { + ClassFactory.clear(); + return new Object(); + } + }); + + callables.addAll(generateCallablesFor(100, DefaultConstructor.class, + NoVisibleConstructor.class, PublicConstructor.class)); + + List> results = executorService.invokeAll(callables); + + for( Future f : results) { + f.get(); // this will throw an Exception if the callable fails for some reason + } + } finally { + executorService.shutdownNow(); + } + + } + + @Test(expected = IllegalStateException.class) + public void testCreate_ConstructorThrowsException() throws Exception { + try { + ClassFactory.create(ConstructorThrowsException.class); + } catch ( IllegalStateException e ) { + assertEquals(e.getCause().getMessage(), "BANG"); + throw e; + } + } + + @Test(expected = RuntimeException.class) + public void testCreate_ConstructorThrowsRuntimeException() { + try { + ClassFactory.create(ConstructorThrowsRuntimeException.class); + } catch ( RuntimeException e ) { + assertEquals(e.getMessage(), "BANG"); + throw e; + } + } + + private List> generateCallablesFor(int count, Class ... classes) { + List> result = new ArrayList>(count); + for( int i = 0 ; i < count ; i ++ ) { + final Class clazz = classes[ i % classes.length ]; + result.add(new Callable() { + public Object call() throws Exception { + return ClassFactory.create(clazz); + } + }); + } + return result; + } +} + +class DefaultConstructor { } + +class NoVisibleConstructor { + + private String foo ; + + private NoVisibleConstructor() { + foo = String.valueOf(System.currentTimeMillis()); + } + + public String getFoo() { + return foo; + } +} + +class PublicConstructor { + + private String foo ; + + + PublicConstructor() { + foo = String.valueOf(System.currentTimeMillis()); + } + + public String getFoo() { + return foo; + } +} + +class ConstructorThrowsException { + + ConstructorThrowsException() throws Exception { + throw new Exception("BANG"); + } +} +class ConstructorThrowsRuntimeException { + + ConstructorThrowsRuntimeException() { + throw new RuntimeException("BANG"); + } +} \ No newline at end of file -- 1.8.4