From fab67b0595bfaddd6692de830c7b54aea782d7b5 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Mon, 30 Jun 2014 00:16:15 +0200 Subject: [PATCH] CachedIntrospectionResults and co consistently avoid synchronization (through use of ConcurrentReferenceHashMap) As a side effect, through ConcurrentReferenceHashMap, we're using soft references for non-cache-safe bean classes. Issue: SPR-11867 --- .../org/springframework/beans/BeanUtils.java | 17 ++- .../beans/CachedIntrospectionResults.java | 102 ++++++++---------- .../GenericTypeAwarePropertyDescriptor.java | 83 +++++++------- .../CachedIntrospectionResultsTests.java | 18 ++-- 4 files changed, 100 insertions(+), 120 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/BeanUtils.java b/spring-beans/src/main/java/org/springframework/beans/BeanUtils.java index 84c8abebf5..e0e4b09ef9 100644 --- a/spring-beans/src/main/java/org/springframework/beans/BeanUtils.java +++ b/spring-beans/src/main/java/org/springframework/beans/BeanUtils.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2013 the original author or authors. + * Copyright 2002-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -29,8 +29,7 @@ import java.util.Collections; import java.util.Date; import java.util.List; import java.util.Locale; -import java.util.Map; -import java.util.WeakHashMap; +import java.util.Set; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -38,6 +37,7 @@ import org.apache.commons.logging.LogFactory; import org.springframework.core.MethodParameter; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; +import org.springframework.util.ConcurrentReferenceHashMap; import org.springframework.util.ReflectionUtils; import org.springframework.util.StringUtils; @@ -57,9 +57,8 @@ public abstract class BeanUtils { private static final Log logger = LogFactory.getLog(BeanUtils.class); - // Effectively using a WeakHashMap as a Set - private static final Map, Boolean> unknownEditorTypes = - Collections.synchronizedMap(new WeakHashMap, Boolean>()); + private static final Set> unknownEditorTypes = + Collections.newSetFromMap(new ConcurrentReferenceHashMap, Boolean>(64)); /** @@ -404,7 +403,7 @@ public abstract class BeanUtils { * @return the corresponding editor, or {@code null} if none found */ public static PropertyEditor findEditorByConvention(Class targetType) { - if (targetType == null || targetType.isArray() || unknownEditorTypes.containsKey(targetType)) { + if (targetType == null || targetType.isArray() || unknownEditorTypes.contains(targetType)) { return null; } ClassLoader cl = targetType.getClassLoader(); @@ -431,7 +430,7 @@ public abstract class BeanUtils { logger.warn("Editor class [" + editorName + "] does not implement [java.beans.PropertyEditor] interface"); } - unknownEditorTypes.put(targetType, Boolean.TRUE); + unknownEditorTypes.add(targetType); return null; } return (PropertyEditor) instantiateClass(editorClass); @@ -441,7 +440,7 @@ public abstract class BeanUtils { logger.debug("No property editor [" + editorName + "] found for type " + targetType.getName() + " according to 'Editor' suffix convention"); } - unknownEditorTypes.put(targetType, Boolean.TRUE); + unknownEditorTypes.add(targetType); return null; } } diff --git a/spring-beans/src/main/java/org/springframework/beans/CachedIntrospectionResults.java b/spring-beans/src/main/java/org/springframework/beans/CachedIntrospectionResults.java index 77175418ae..38bc83c901 100644 --- a/spring-beans/src/main/java/org/springframework/beans/CachedIntrospectionResults.java +++ b/spring-beans/src/main/java/org/springframework/beans/CachedIntrospectionResults.java @@ -20,15 +20,12 @@ import java.beans.BeanInfo; import java.beans.IntrospectionException; import java.beans.Introspector; import java.beans.PropertyDescriptor; -import java.lang.ref.Reference; -import java.lang.ref.WeakReference; -import java.util.HashSet; +import java.util.Collections; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.WeakHashMap; import java.util.concurrent.ConcurrentHashMap; import org.apache.commons.logging.Log; @@ -38,6 +35,7 @@ import org.springframework.core.SpringProperties; import org.springframework.core.convert.TypeDescriptor; import org.springframework.core.io.support.SpringFactoriesLoader; import org.springframework.util.ClassUtils; +import org.springframework.util.ConcurrentReferenceHashMap; import org.springframework.util.StringUtils; /** @@ -107,14 +105,22 @@ public class CachedIntrospectionResults { * Set of ClassLoaders that this CachedIntrospectionResults class will always * accept classes from, even if the classes do not qualify as cache-safe. */ - static final Set acceptedClassLoaders = new HashSet(); + static final Set acceptedClassLoaders = + Collections.newSetFromMap(new ConcurrentHashMap(16)); /** - * Map keyed by class containing CachedIntrospectionResults. - * Needs to be a WeakHashMap with WeakReferences as values to allow - * for proper garbage collection in case of multiple class loaders. + * Map keyed by Class containing CachedIntrospectionResults, strongly held. + * This variant is being used for cache-safe bean classes. */ - static final Map, Object> classCache = new WeakHashMap, Object>(); + static final Map, CachedIntrospectionResults> strongClassCache = + new ConcurrentHashMap, CachedIntrospectionResults>(64); + + /** + * Map keyed by Class containing CachedIntrospectionResults, softly held. + * This variant is being used for non-cache-safe bean classes. + */ + static final Map, CachedIntrospectionResults> softClassCache = + new ConcurrentReferenceHashMap, CachedIntrospectionResults>(64); /** @@ -131,9 +137,7 @@ public class CachedIntrospectionResults { */ public static void acceptClassLoader(ClassLoader classLoader) { if (classLoader != null) { - synchronized (acceptedClassLoaders) { - acceptedClassLoaders.add(classLoader); - } + acceptedClassLoaders.add(classLoader); } } @@ -144,20 +148,22 @@ public class CachedIntrospectionResults { * @param classLoader the ClassLoader to clear the cache for */ public static void clearClassLoader(ClassLoader classLoader) { - synchronized (classCache) { - for (Iterator> it = classCache.keySet().iterator(); it.hasNext();) { - Class beanClass = it.next(); - if (isUnderneathClassLoader(beanClass.getClassLoader(), classLoader)) { - it.remove(); - } + for (Iterator it = acceptedClassLoaders.iterator(); it.hasNext();) { + ClassLoader registeredLoader = it.next(); + if (isUnderneathClassLoader(registeredLoader, classLoader)) { + it.remove(); } } - synchronized (acceptedClassLoaders) { - for (Iterator it = acceptedClassLoaders.iterator(); it.hasNext();) { - ClassLoader registeredLoader = it.next(); - if (isUnderneathClassLoader(registeredLoader, classLoader)) { - it.remove(); - } + for (Iterator> it = strongClassCache.keySet().iterator(); it.hasNext();) { + Class beanClass = it.next(); + if (isUnderneathClassLoader(beanClass.getClassLoader(), classLoader)) { + it.remove(); + } + } + for (Iterator> it = softClassCache.keySet().iterator(); it.hasNext();) { + Class beanClass = it.next(); + if (isUnderneathClassLoader(beanClass.getClassLoader(), classLoader)) { + it.remove(); } } } @@ -170,35 +176,25 @@ public class CachedIntrospectionResults { */ @SuppressWarnings("unchecked") static CachedIntrospectionResults forClass(Class beanClass) throws BeansException { - CachedIntrospectionResults results; - Object value; - synchronized (classCache) { - value = classCache.get(beanClass); + CachedIntrospectionResults results = strongClassCache.get(beanClass); + if (results != null) { + return results; } - if (value instanceof Reference) { - Reference ref = (Reference) value; - results = ref.get(); + results = softClassCache.get(beanClass); + if (results != null) { + return results; } - else { - results = (CachedIntrospectionResults) value; + + results = new CachedIntrospectionResults(beanClass); + if (ClassUtils.isCacheSafe(beanClass, CachedIntrospectionResults.class.getClassLoader()) || + isClassLoaderAccepted(beanClass.getClassLoader())) { + strongClassCache.put(beanClass, results); } - if (results == null) { - if (ClassUtils.isCacheSafe(beanClass, CachedIntrospectionResults.class.getClassLoader()) || - isClassLoaderAccepted(beanClass.getClassLoader())) { - results = new CachedIntrospectionResults(beanClass); - synchronized (classCache) { - classCache.put(beanClass, results); - } - } - else { - if (logger.isDebugEnabled()) { - logger.debug("Not strongly caching class [" + beanClass.getName() + "] because it is not cache-safe"); - } - results = new CachedIntrospectionResults(beanClass); - synchronized (classCache) { - classCache.put(beanClass, new WeakReference(results)); - } + else { + if (logger.isDebugEnabled()) { + logger.debug("Not strongly caching class [" + beanClass.getName() + "] because it is not cache-safe"); } + softClassCache.put(beanClass, results); } return results; } @@ -211,13 +207,7 @@ public class CachedIntrospectionResults { * @see #acceptClassLoader */ private static boolean isClassLoaderAccepted(ClassLoader classLoader) { - // Iterate over array copy in order to avoid synchronization for the entire - // ClassLoader check (avoiding a synchronized acceptedClassLoaders Iterator). - ClassLoader[] acceptedLoaderArray; - synchronized (acceptedClassLoaders) { - acceptedLoaderArray = acceptedClassLoaders.toArray(new ClassLoader[acceptedClassLoaders.size()]); - } - for (ClassLoader acceptedLoader : acceptedLoaderArray) { + for (ClassLoader acceptedLoader : acceptedClassLoaders) { if (isUnderneathClassLoader(classLoader, acceptedLoader)) { return true; } diff --git a/spring-beans/src/main/java/org/springframework/beans/GenericTypeAwarePropertyDescriptor.java b/spring-beans/src/main/java/org/springframework/beans/GenericTypeAwarePropertyDescriptor.java index 096bae5a61..be6f25e6af 100644 --- a/spring-beans/src/main/java/org/springframework/beans/GenericTypeAwarePropertyDescriptor.java +++ b/spring-beans/src/main/java/org/springframework/beans/GenericTypeAwarePropertyDescriptor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2013 the original author or authors. + * Copyright 2002-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -42,18 +42,18 @@ class GenericTypeAwarePropertyDescriptor extends PropertyDescriptor { private final Class beanClass; + private final Class propertyEditorClass; + private final Method readMethod; private final Method writeMethod; - private final Class propertyEditorClass; - private volatile Set ambiguousWriteMethods; - private Class propertyType; - private MethodParameter writeMethodParameter; + private Class propertyType; + public GenericTypeAwarePropertyDescriptor(Class beanClass, String propertyName, Method readMethod, Method writeMethod, Class propertyEditorClass) @@ -78,27 +78,44 @@ class GenericTypeAwarePropertyDescriptor extends PropertyDescriptor { this.readMethod = readMethodToUse; this.writeMethod = writeMethodToUse; - if (this.writeMethod != null && this.readMethod == null) { - // Write method not matched against read method: potentially ambiguous through - // several overloaded variants, in which case an arbitrary winner has been chosen - // by the JDK's JavaBeans Introspector... - Set ambiguousCandidates = new HashSet(); - for (Method method : beanClass.getMethods()) { - if (method.getName().equals(writeMethodToUse.getName()) && - !method.equals(writeMethodToUse) && !method.isBridge()) { - ambiguousCandidates.add(method); + if (this.writeMethod != null) { + if (this.readMethod == null) { + // Write method not matched against read method: potentially ambiguous through + // several overloaded variants, in which case an arbitrary winner has been chosen + // by the JDK's JavaBeans Introspector... + Set ambiguousCandidates = new HashSet(); + for (Method method : beanClass.getMethods()) { + if (method.getName().equals(writeMethodToUse.getName()) && + !method.equals(writeMethodToUse) && !method.isBridge()) { + ambiguousCandidates.add(method); + } + } + if (!ambiguousCandidates.isEmpty()) { + this.ambiguousWriteMethods = ambiguousCandidates; } } - if (!ambiguousCandidates.isEmpty()) { - this.ambiguousWriteMethods = ambiguousCandidates; - } + this.writeMethodParameter = new MethodParameter(this.writeMethod, 0); + GenericTypeResolver.resolveParameterType(this.writeMethodParameter, this.beanClass); + } + + if (this.readMethod != null) { + this.propertyType = GenericTypeResolver.resolveReturnType(this.readMethod, this.beanClass); + } + else if (this.writeMethodParameter != null) { + this.propertyType = this.writeMethodParameter.getParameterType(); } } + public Class getBeanClass() { return this.beanClass; } + @Override + public Class getPropertyEditorClass() { + return this.propertyEditorClass; + } + @Override public Method getReadMethod() { return this.readMethod; @@ -120,39 +137,13 @@ class GenericTypeAwarePropertyDescriptor extends PropertyDescriptor { return this.writeMethod; } - @Override - public Class getPropertyEditorClass() { - return this.propertyEditorClass; + public MethodParameter getWriteMethodParameter() { + return this.writeMethodParameter; } @Override - public synchronized Class getPropertyType() { - if (this.propertyType == null) { - if (this.readMethod != null) { - this.propertyType = GenericTypeResolver.resolveReturnType(this.readMethod, this.beanClass); - } - else { - MethodParameter writeMethodParam = getWriteMethodParameter(); - if (writeMethodParam != null) { - this.propertyType = writeMethodParam.getParameterType(); - } - else { - this.propertyType = super.getPropertyType(); - } - } - } + public Class getPropertyType() { return this.propertyType; } - public synchronized MethodParameter getWriteMethodParameter() { - if (this.writeMethod == null) { - return null; - } - if (this.writeMethodParameter == null) { - this.writeMethodParameter = new MethodParameter(this.writeMethod, 0); - GenericTypeResolver.resolveParameterType(this.writeMethodParameter, this.beanClass); - } - return this.writeMethodParameter; - } - } diff --git a/spring-beans/src/test/java/org/springframework/beans/CachedIntrospectionResultsTests.java b/spring-beans/src/test/java/org/springframework/beans/CachedIntrospectionResultsTests.java index 618ced2e49..e0901542d0 100644 --- a/spring-beans/src/test/java/org/springframework/beans/CachedIntrospectionResultsTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/CachedIntrospectionResultsTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2013 the original author or authors. + * Copyright 2002-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -33,35 +33,35 @@ import static org.junit.Assert.*; * @author Chris Beams * @author Arjen Poutsma */ -public final class CachedIntrospectionResultsTests { +public class CachedIntrospectionResultsTests { @Test public void acceptAndClearClassLoader() throws Exception { BeanWrapper bw = new BeanWrapperImpl(TestBean.class); assertTrue(bw.isWritableProperty("name")); assertTrue(bw.isWritableProperty("age")); - assertTrue(CachedIntrospectionResults.classCache.containsKey(TestBean.class)); + assertTrue(CachedIntrospectionResults.strongClassCache.containsKey(TestBean.class)); ClassLoader child = new OverridingClassLoader(getClass().getClassLoader()); Class tbClass = child.loadClass("org.springframework.tests.sample.beans.TestBean"); - assertFalse(CachedIntrospectionResults.classCache.containsKey(tbClass)); + assertFalse(CachedIntrospectionResults.strongClassCache.containsKey(tbClass)); CachedIntrospectionResults.acceptClassLoader(child); bw = new BeanWrapperImpl(tbClass); assertTrue(bw.isWritableProperty("name")); assertTrue(bw.isWritableProperty("age")); - assertTrue(CachedIntrospectionResults.classCache.containsKey(tbClass)); + assertTrue(CachedIntrospectionResults.strongClassCache.containsKey(tbClass)); CachedIntrospectionResults.clearClassLoader(child); - assertFalse(CachedIntrospectionResults.classCache.containsKey(tbClass)); + assertFalse(CachedIntrospectionResults.strongClassCache.containsKey(tbClass)); - assertTrue(CachedIntrospectionResults.classCache.containsKey(TestBean.class)); + assertTrue(CachedIntrospectionResults.strongClassCache.containsKey(TestBean.class)); } @Test public void clearClassLoaderForSystemClassLoader() throws Exception { BeanUtils.getPropertyDescriptors(ArrayList.class); - assertTrue(CachedIntrospectionResults.classCache.containsKey(ArrayList.class)); + assertTrue(CachedIntrospectionResults.strongClassCache.containsKey(ArrayList.class)); CachedIntrospectionResults.clearClassLoader(ArrayList.class.getClassLoader()); - assertFalse(CachedIntrospectionResults.classCache.containsKey(ArrayList.class)); + assertFalse(CachedIntrospectionResults.strongClassCache.containsKey(ArrayList.class)); } @Test