Revise cache safety check to avoid performance regression in EAR packaged applications on WildFly [SPR-16714]

[spring-projects-issues]

Diego Pettisani opened SPR-16714 and commented

We had a performance issue after upgraded from Spring 3.2.2 to 3.2.16.

Our application is packaged in an EAR file having several "skinny" WARs, so the content inside the EAR is:

.
|-- lib
|   -- spring-core-3.2.16.jar
|   -- spring-beans-3.2.16.jar
|   -- company-platform.jar
|   -- etc. etc.
|-- META-INF
|   -- application.xml
|-- app1.war
|   -- WEB-INF
|      -- web.xml
|      -- webapp-beans.xml
|-- app2.war
|   -- WEB-INF
|      -- web.xml
|      -- webapp-beans.xml
| -- more wars.....
.

In each web.xml file we have this configuration:

...
<context-param>
	<param-name>contextConfigLocation</param-name>
	<param-value>WEB-INF/webapp-beans.xml</param-value>
</context-param>
<context-param>
	<param-name>parentContextKey</param-name>
	<param-value>ear.context</param-value>
</context-param>
<listener>
	<listener-class>org.springframework.web.context.ContextLoaderListener</listener-class>
</listener>
...

The parent context has several hundred of beans.

We developed an internal platform for our applications that often calls the following BeanFactory method:

org.springframework.beans.factory.BeanFactory.getBean(Class)

In turn, The above getBean(Class) method calls the following DefaultListableBeanFactory method:

org.springframework.beans.factory.support.DefaultListableBeanFactory.getBeanNamesForType(Class<?>, boolean, boolean)

After upgraded to Spring 3.2.16 the call to the above getBeanNamesForType() method was very slow and CPU consuming. The root cause is the following class loader check:

if (ClassUtils.isCacheSafe(type, getBeanClassLoader())) {
	cache.put(type, resolvedBeanNames);
}

added for fixing a memory leak highlighted in the #16145 and released in Spring 3.2.9.

The check inside the if condition:

ClassUtils.isCacheSafe(type, getBeanClassLoader())

returns always false because:

• type is an object of the EAR class loader
• getBeanClassLoader() returns always the WAR class loader
• the EAR class loader and the WAR class loader are different and they are sibling in the class loader hierarchy

So the following internal caches of the DefaultListableBeanFactory class:

/** Map of singleton and non-singleton bean names, keyed by dependency type */
private final Map<Class<?>, String[]> allBeanNamesByType = new ConcurrentHashMap<Class<?>, String[]>(64);

/** Map of singleton-only bean names, keyed by dependency type */
private final Map<Class<?>, String[]> singletonBeanNamesByType = new ConcurrentHashMap<Class<?>, String[]>(64);

will always be empty and they will never be used.

Furthermore informations:

• The application server used is Wildfly 10.0.0.Final
• The JDK used is JDK8 update 74.
• I supposed that upgrading our application to the Spring 4.x or 5.x version will not fix the performance regression because the class loader check is still there.

---

Affects: 3.2.18, 4.3.16, 5.0.5

Attachments:

• SPR_16714_short_term_solution_for_Spring_3.2.16.zip (3.26 kB)

Issue Links:

• Memory leak when using annotation based auto-wiring in child context [SPR-11520] #16145 Memory leak when using annotation based auto-wiring in child context

Referenced from: commits 46e3a91, 2989f01, 0efa7a0, 295929c

Backported to: 4.3.17

[spring-projects-issues]

Juergen Hoeller commented

We'll see what we can do about this. Please note that the 3.2.x line at its end-of-life already, so potential measures would only be applied to the upcoming 4.3.17 and 5.0.6 releases.

[spring-projects-issues]

Diego Pettisani commented

Thanks Jurgen for the quick response and for having accepted the issue.

Our program already planned a migration to Spring 4, so a fix for that streams will be suitable for us too.

As short-term solution for an upcoming release we are thinking to patch the DefaultListableBeanFactory class by removing that if condition. Parent and child contexts have the same lifecycle in our app, so we should not suffer of the memory leak raised in the #16145.

Of course, the patch will be our last option and a temporary solution.

[spring-projects-issues]

Juergen Hoeller commented

This seems to be rather specific to WildFly since it is generally unusual to have a valid Class not loaded by the current ClassLoader or an ancestor thereof. Could you share some details about the actual ClassLoader layout in your runtime environment? What does the hierarchy look like if class loaders are siblings but nevertheless sharing their class space?

In terms of potential solutions, we could consider inverting our cache-safety algorithm to only consider a Class as non-cache-safe if it has been loaded by a child of the current ClassLoader (catching the intention behind #16145 from a different angle) but leniently accepting other scenarios as safe, including a non-traditional layout like in WildFly.

[spring-projects-issues]

Juergen Hoeller commented

I eventually went with a combination of the original parent traversal and the inverted algorithm, being able to clearly and efficiently identify many common true and false scenarios. If we remain undecided, we compare whether the same Class can be loaded in the given ClassLoader and consider it cache-safe then as well. I assume this will cover WildFly's layout accordingly (independent from its hierarchy arrangement) and will still be reasonably efficient since we're only going to perform the more expensive fallback check once per cache put attempt but not on retrieval. The tradeoff is a bit of extra expense when it turns out not to be cache-safe eventually... but that goes with a lot of extra expense freshly retrieving the affected metadata in any case, so it shouldn't really cause any extra pain.

[spring-projects-issues]

Diego Pettisani commented

Thanks Juergen, we are going forward to download the next Spring 4 release. I am sure that our program will accelerate the migration from Spring 3.2.x to Spring 4.3.x.

For a very short-term solution to implement in the our Spring 3.2.16 app we extended the XmlWebApplicationContext and the ClassPathXmlApplicationContext in order to create a subclass of DefaultListableBeanFactory that overrides the method getBeanNamesForType having the implementation without the cache safe check:

public class CustomListableBeanFactory extends DefaultListableBeanFactory {

	private final Map<Class<?>, String[]> allBeanNamesByType = 
                    new ConcurrentHashMap<Class<?>, String[]>(64);
	private final Map<Class<?>, String[]> singletonBeanNamesByType = 
                    new ConcurrentHashMap<Class<?>, String[]>(64);

	public CustomListableBeanFactory() {
		super();
	}


	public CustomListableBeanFactory(BeanFactory parentBeanFactory) {
		super(parentBeanFactory);
	}

	@Override
	public String[] getBeanNamesForType(Class<?> type, boolean includeNonSingletons, boolean allowEagerInit) {
		if (!isConfigurationFrozen() || type == null || !allowEagerInit) {
			return super.getBeanNamesForType(type, includeNonSingletons, allowEagerInit);
		}
		Map<Class<?>, String[]> cache = (includeNonSingletons ? this.allBeanNamesByType
				: this.singletonBeanNamesByType);
		String[] resolvedBeanNames = cache.get(type);
		if (resolvedBeanNames != null) {
			return resolvedBeanNames;
		}
		resolvedBeanNames = super.getBeanNamesForType(type, includeNonSingletons, allowEagerInit);
		cache.put(type, resolvedBeanNames);
		return resolvedBeanNames;
	}

	@Override
	public void registerSingleton(String beanName, Object singletonObject) throws IllegalStateException {
		super.registerSingleton(beanName, singletonObject);
		clearByTypeCache();
	}

	@Override
	public void destroySingleton(String beanName) {
		super.destroySingleton(beanName);
		clearByTypeCache();
	}

	private void clearByTypeCache() {
		this.allBeanNamesByType.clear();
		this.singletonBeanNamesByType.clear();
	}
}

By using this implementation, potentially, we could suffer of the memory leak raised in the #16145 but we never never never do an hot-deploy of an application having a child Spring context, so we should be safe for that memory leak. Do you agree?

I attached the sources that we will use for the temporary solution in our Spring 3.2.16 app, in the meanwhile we will plan the Spring 4.3.x migration.