From db1cb13448dcdc11c6ebd087d0a436614d541d90 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Sun, 27 May 2012 16:27:35 +0300 Subject: [PATCH 1/2] Polish Issue: SPR-6870 --- .../support/DefaultListableBeanFactory.java | 45 ++++++++++--------- .../DefaultListableBeanFactoryTests.java | 28 ++++++------ 2 files changed, 38 insertions(+), 35 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java index 6f7e75c9e58..c4f53525306 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java @@ -21,13 +21,14 @@ import java.io.NotSerializableException; import java.io.ObjectInputStream; import java.io.ObjectStreamException; import java.io.Serializable; + import java.lang.annotation.Annotation; import java.lang.ref.Reference; import java.lang.ref.WeakReference; -import java.lang.reflect.ParameterizedType; -import java.lang.reflect.Type; + import java.security.AccessController; import java.security.PrivilegedAction; + import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -38,6 +39,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; + import javax.inject.Provider; import org.springframework.beans.BeansException; @@ -98,7 +100,7 @@ import org.springframework.util.StringUtils; public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFactory implements ConfigurableListableBeanFactory, BeanDefinitionRegistry, Serializable { - private static Class javaxInjectProviderClass = null; + private static Class javaxInjectProviderClass = null; static { ClassLoader cl = DefaultListableBeanFactory.class.getClassLoader(); @@ -128,7 +130,7 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto private AutowireCandidateResolver autowireCandidateResolver = new SimpleAutowireCandidateResolver(); /** Map from dependency type to corresponding autowired value */ - private final Map resolvableDependencies = new HashMap(); + private final Map, Object> resolvableDependencies = new HashMap, Object>(); /** Map of bean definition objects, keyed by bean name */ private final Map beanDefinitionMap = new ConcurrentHashMap(); @@ -294,11 +296,11 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto } } - public String[] getBeanNamesForType(Class type) { + public String[] getBeanNamesForType(Class type) { return getBeanNamesForType(type, true, true); } - public String[] getBeanNamesForType(Class type, boolean includeNonSingletons, boolean allowEagerInit) { + public String[] getBeanNamesForType(Class type, boolean includeNonSingletons, boolean allowEagerInit) { List result = new ArrayList(); // Check all bean definitions. @@ -441,7 +443,7 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto */ public A findAnnotationOnBean(String beanName, Class annotationType) { A ann = null; - Class beanType = getType(beanName); + Class beanType = getType(beanName); if (beanType != null) { ann = AnnotationUtils.findAnnotation(beanType, annotationType); } @@ -564,18 +566,18 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto RootBeanDefinition bd = getMergedLocalBeanDefinition(beanName); if (!bd.isAbstract() && bd.isSingleton() && !bd.isLazyInit()) { if (isFactoryBean(beanName)) { - final FactoryBean factory = (FactoryBean) getBean(FACTORY_BEAN_PREFIX + beanName); + final FactoryBean factory = (FactoryBean) getBean(FACTORY_BEAN_PREFIX + beanName); boolean isEagerInit; if (System.getSecurityManager() != null && factory instanceof SmartFactoryBean) { isEagerInit = AccessController.doPrivileged(new PrivilegedAction() { public Boolean run() { - return ((SmartFactoryBean) factory).isEagerInit(); + return ((SmartFactoryBean) factory).isEagerInit(); } }, getAccessControlContext()); } else { isEagerInit = (factory instanceof SmartFactoryBean && - ((SmartFactoryBean) factory).isEagerInit()); + ((SmartFactoryBean) factory).isEagerInit()); } if (isEagerInit) { getBean(beanName); @@ -723,7 +725,7 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto } if (type.isArray()) { - Class componentType = type.getComponentType(); + Class componentType = type.getComponentType(); Map matchingBeans = findAutowireCandidates(beanName, componentType, descriptor); if (matchingBeans.isEmpty()) { if (descriptor.isRequired()) { @@ -738,7 +740,7 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto return converter.convertIfNecessary(matchingBeans.values(), type); } else if (Collection.class.isAssignableFrom(type) && type.isInterface()) { - Class elementType = descriptor.getCollectionType(); + Class elementType = descriptor.getCollectionType(); if (elementType == null) { if (descriptor.isRequired()) { throw new FatalBeanException("No element type declared for collection [" + type.getName() + "]"); @@ -759,7 +761,7 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto return converter.convertIfNecessary(matchingBeans.values(), type); } else if (Map.class.isAssignableFrom(type) && type.isInterface()) { - Class keyType = descriptor.getMapKeyType(); + Class keyType = descriptor.getMapKeyType(); if (keyType == null || !String.class.isAssignableFrom(keyType)) { if (descriptor.isRequired()) { throw new FatalBeanException("Key type [" + keyType + "] of map [" + type.getName() + @@ -767,7 +769,7 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto } return null; } - Class valueType = descriptor.getMapValueType(); + Class valueType = descriptor.getMapValueType(); if (valueType == null) { if (descriptor.isRequired()) { throw new FatalBeanException("No value type declared for map [" + type.getName() + "]"); @@ -828,12 +830,12 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto * @see #autowireConstructor */ protected Map findAutowireCandidates( - String beanName, Class requiredType, DependencyDescriptor descriptor) { + String beanName, Class requiredType, DependencyDescriptor descriptor) { String[] candidateNames = BeanFactoryUtils.beanNamesForTypeIncludingAncestors( this, requiredType, true, descriptor.isEager()); Map result = new LinkedHashMap(candidateNames.length); - for (Class autowiringType : this.resolvableDependencies.keySet()) { + for (Class autowiringType : this.resolvableDependencies.keySet()) { if (autowiringType.isAssignableFrom(requiredType)) { Object autowiringValue = this.resolvableDependencies.get(autowiringType); autowiringValue = AutowireUtils.resolveAutowiringValue(autowiringValue, requiredType); @@ -918,7 +920,7 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto * Raise a NoSuchBeanDefinitionException for an unresolvable dependency. */ private void raiseNoSuchBeanDefinitionException( - Class type, String dependencyDescription, DependencyDescriptor descriptor) + Class type, String dependencyDescription, DependencyDescriptor descriptor) throws NoSuchBeanDefinitionException { throw new NoSuchBeanDefinitionException(type, dependencyDescription, @@ -967,6 +969,7 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto * Minimal id reference to the factory. * Resolved to the actual factory instance on deserialization. */ + @SuppressWarnings("serial") private static class SerializedBeanFactoryReference implements Serializable { private final String id; @@ -976,7 +979,7 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto } private Object readResolve() { - Reference ref = serializableFactories.get(this.id); + Reference ref = serializableFactories.get(this.id); if (ref == null) { throw new IllegalStateException( "Cannot deserialize BeanFactory with id " + this.id + ": no factory registered for this id"); @@ -994,7 +997,8 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto /** * Serializable ObjectFactory for lazy resolution of a dependency. */ - private class DependencyObjectFactory implements ObjectFactory, Serializable { + @SuppressWarnings("serial") + private class DependencyObjectFactory implements ObjectFactory, Serializable { private final DependencyDescriptor descriptor; @@ -1015,7 +1019,8 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto /** * Serializable ObjectFactory for lazy resolution of a dependency. */ - private class DependencyProvider extends DependencyObjectFactory implements Provider { + @SuppressWarnings("serial") + private class DependencyProvider extends DependencyObjectFactory implements Provider { public DependencyProvider(DependencyDescriptor descriptor, String beanName) { super(descriptor, beanName); diff --git a/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java b/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java index 25d07a81015..8505da7cba1 100644 --- a/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 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. @@ -16,26 +16,18 @@ package org.springframework.beans.factory; -import static org.hamcrest.CoreMatchers.is; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNotSame; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertSame; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; -import static org.springframework.beans.factory.support.BeanDefinitionBuilder.rootBeanDefinition; - import java.lang.reflect.Field; + import java.net.MalformedURLException; + import java.security.AccessControlContext; import java.security.AccessController; import java.security.Principal; import java.security.PrivilegedAction; + import java.text.NumberFormat; import java.text.ParseException; + import java.util.Arrays; import java.util.Iterator; import java.util.List; @@ -48,8 +40,10 @@ import javax.security.auth.Subject; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; + import org.junit.Ignore; import org.junit.Test; + import org.springframework.beans.BeansException; import org.springframework.beans.MutablePropertyValues; import org.springframework.beans.NotWritablePropertyException; @@ -92,6 +86,10 @@ import test.beans.LifecycleBean; import test.beans.NestedTestBean; import test.beans.TestBean; +import static org.hamcrest.CoreMatchers.*; + +import static org.junit.Assert.*; + /** * Tests properties population and autowire behavior. * @@ -2159,8 +2157,8 @@ public class DefaultListableBeanFactoryTests { @Test public void testContainsBeanReturnsTrueEvenForAbstractBeanDefinition() { DefaultListableBeanFactory bf = new DefaultListableBeanFactory(); - bf.registerBeanDefinition("abs", - rootBeanDefinition(TestBean.class).setAbstract(true).getBeanDefinition()); + bf.registerBeanDefinition("abs", BeanDefinitionBuilder + .rootBeanDefinition(TestBean.class).setAbstract(true).getBeanDefinition()); assertThat(bf.containsBean("abs"), is(true)); assertThat(bf.containsBean("bogus"), is(false)); } From 4c7a1c0a5403b35dd812dae1f2a753538928bb32 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Sun, 27 May 2012 17:40:33 +0300 Subject: [PATCH 2/2] Cache by-type lookups in DefaultListableBeanFactory Prior to this change, by-type lookups using DLBF#getBeanNamesForType required traversal of all bean definitions within the bean factory in order to inspect their bean class for assignability to the target type. These operations are comparatively expensive and when there are a large number of beans registered within the container coupled with a large number of by-type lookups at runtime, the performance impact can be severe. The test introduced here demonstrates such a scenario clearly. This performance problem is likely to manifest in large Spring-based applications using non-singleton beans, particularly request-scoped beans that may be created and wired many thousands of times per second. This commit introduces a simple ConcurrentHashMap-based caching strategy for by-type lookups; container-wide assignability checks happen only once on the first by-type lookup and are afterwards cached by type with the values in the map being an array of all bean names assignable to that type. This means that at runtime when creating and autowiring non-singleton beans, the cost of by-type lookups is reduced to that of ConcurrentHashMap#get. Issue: SPR-6870 --- .../support/DefaultListableBeanFactory.java | 26 +++++++++++++++++ .../DefaultListableBeanFactoryTests.java | 28 +++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java index c4f53525306..afabaac09df 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java @@ -92,6 +92,7 @@ import org.springframework.util.StringUtils; * @author Juergen Hoeller * @author Sam Brannen * @author Costin Leau + * @author Chris Beams * @since 16 April 2001 * @see StaticListableBeanFactory * @see PropertiesBeanDefinitionReader @@ -135,6 +136,12 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto /** Map of bean definition objects, keyed by bean name */ private final Map beanDefinitionMap = new ConcurrentHashMap(); + /** Map of singleton bean names keyed by bean class */ + private final Map, String[]> singletonBeanNamesByType = new ConcurrentHashMap, String[]>(); + + /** Map of non-singleton bean names keyed by bean class */ + private final Map, String[]> nonSingletonBeanNamesByType = new ConcurrentHashMap, String[]>(); + /** List of bean definition names, in registration order */ private final List beanDefinitionNames = new ArrayList(); @@ -301,6 +308,21 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto } public String[] getBeanNamesForType(Class type, boolean includeNonSingletons, boolean allowEagerInit) { + if (type == null || !allowEagerInit) { + return this.doGetBeanNamesForType(type, includeNonSingletons, allowEagerInit); + } + Map, String[]> cache = includeNonSingletons ? + this.nonSingletonBeanNamesByType : this.singletonBeanNamesByType; + String[] resolvedBeanNames = cache.get(type); + if (resolvedBeanNames != null) { + return resolvedBeanNames; + } + resolvedBeanNames = this.doGetBeanNamesForType(type, includeNonSingletons, allowEagerInit); + cache.put(type, resolvedBeanNames); + return resolvedBeanNames; + } + + private String[] doGetBeanNamesForType(Class type, boolean includeNonSingletons, boolean allowEagerInit) { List result = new ArrayList(); // Check all bean definitions. @@ -671,6 +693,10 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto destroySingleton(beanName); } + // Remove any assumptions about by-type mappings + this.singletonBeanNamesByType.clear(); + this.nonSingletonBeanNamesByType.clear(); + // Reset all bean definitions that have the given bean as parent (recursively). for (String bdName : this.beanDefinitionNames) { if (!beanName.equals(bdName)) { diff --git a/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java b/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java index 8505da7cba1..c123347482c 100644 --- a/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java @@ -2164,6 +2164,34 @@ public class DefaultListableBeanFactoryTests { } + static class A { } + static class B { } + + /** + * Test that by-type bean lookup caching is working effectively by searching for a + * bean of type B 10K times within a container having 1K additional beans of type A. + * Prior to by-type caching, each bean lookup would traverse the entire container + * (all 1001 beans), performing expensive assignability checks, etc. Now these + * operations are necessary only once, providing a dramatic performance improvement. + * On load-free modern hardware (e.g. an 8-core MPB), this method should complete well + * under the 1000 ms timeout, usually ~= 300ms. With caching removed and on the same + * hardware the method will take ~13000 ms. See SPR-6870. + */ + @Test(timeout=1000) + public void testByTypeLookupIsFastEnough() { + DefaultListableBeanFactory bf = new DefaultListableBeanFactory(); + + for (int i=0; i<1000; i++) { + bf.registerBeanDefinition("a"+i, new RootBeanDefinition(A.class)); + } + bf.registerBeanDefinition("b", new RootBeanDefinition(B.class)); + + for (int i=0; i<10000; i++) { + bf.getBean(B.class); + } + } + + public static class NoDependencies { private NoDependencies() {