[Performance] CachedIntrospectionResults has wrong approach to caching. [SPR-4876]

[spring-projects-issues]

Tomasz Wysocki opened SPR-4876 and commented

Current approach in CachedIntrospectionResults is to cache "weakly" classes that are supposed to be "not cache-safe".
This unfortunately causes that "weakly" cached introspection results can expire on gc.
Rebuilding cached introspection results is costly in environments where there are lots of classes on a classpath.
What is more rebuilding cause multi-threaded applications to synchronize on rebuilding. That cause significant drops in performance for periods of rebuilding which is not acceptable.
This had been observed with JMeter running against some portal installation and diagnosed with YJP 7.5 profiler.

Maybe it is not common approach to put spring jars on a server classpath, but it will become more and more common with environments (OSGi for instance) where spring is a part of a platform.
So putting spring jar on a server classpath cause all application classes not to be cache-safe.
Workaround that forces applications to register it classloaders as cache-safe explicitly does seem a wrong solution for a problem to me.

I propose a different approach where unloading of cached introspection results is triggered by unloading a classloader.
It has two of benefits compared to existing solution.

1. Application do not need to register and unregister their classloaders.
2. Introspection results are persisted as long as application is loaded and do not expire on GC.
3. Cached introspection results expire when application is unloaded.

Modified CachedIntrospectionResults class from 2.0 version renamed to CachedIntrospectionResults2 follows as proposed solution.
Note that I've decided to use 2.0 version because I see improvements done in later version as a step in a wrong direction.
As you can see implementation is straight-forward and does not require any registration/unregistration of classloaders, and isCacheSafe method had been removed entirely (all classes are regarded as cache safe).

/*
 * Copyright 2002-2006 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.
 * 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.springframework.beans;

import java.beans.BeanInfo;
import java.beans.IntrospectionException;
import java.beans.Introspector;
import java.beans.PropertyDescriptor;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.WeakHashMap;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

/**
 * Class to cache PropertyDescriptor information for a Java class.
 * Package-visible; not for use by application code.
 *
 * <p>Necessary as <code>Introspector.getBeanInfo()</code> in JDK 1.3 will return a
 * new deep copy of the BeanInfo every time we ask for it. We take the opportunity
 * to cache property descriptors by method name for fast lookup. Furthermore,
 * we do our own caching of descriptors here, rather than rely on the JDK's
 * system-wide BeanInfo cache (to avoid leaks on class loader shutdown).
 *
 * <p>Information is cached statically, so we don't need to create new
 * objects of this class for every JavaBean we manipulate. Hence, this class
 * implements the factory design pattern, using a private constructor
 * and a static <code>forClass</code> method to obtain instances.
 *
 * @author Rod Johnson
 * @author Juergen Hoeller
 * @since 05 May 2001
 * @see #forClass(Class)
 */
public final class CachedIntrospectionResults2 {

	private static final Log logger = LogFactory.getLog(CachedIntrospectionResults2.class);

	/**
	 * Map keyed by classLoader containing Map of CachedIntrospectionResults.
	 * Needs to be a WeakHashMap with WeakReferences as values to allow
	 * for proper garbage collection in case of multiple class loaders.
	 */
	public static final Map classLoaderCache = Collections.synchronizedMap(new WeakHashMap());


	/**
	 * Create CachedIntrospectionResults for the given bean class.
	 * <P>We don't want to use synchronization here. Object references are atomic,
	 * so we can live with doing the occasional unnecessary lookup at startup only.
	 * @param beanClass the bean class to analyze
	 */
	public static CachedIntrospectionResults2 forClass(Class beanClass) throws BeansException {
		CachedIntrospectionResults2 results = null;
		ClassLoader cl = beanClass.getClassLoader();
		Map classCache = (Map) classLoaderCache.get(cl);
		if (classCache != null) {
			results = (CachedIntrospectionResults2) classCache.get(beanClass);
		}
		if (results == null) {
			// can throw BeansException
			results = new CachedIntrospectionResults2(beanClass);
			if (classCache == null) {
				classCache = Collections.synchronizedMap(new WeakHashMap());
				classLoaderCache.put(cl, classCache);
			}
			classCache.put(beanClass, results);
		}
		else {
			if (logger.isDebugEnabled()) {
				logger.debug("Using cached introspection results for class [" + beanClass.getName() + "]");
			}
		}
		return results;
	}

	private final BeanInfo beanInfo;

	/** Property descriptors keyed by property name */
	private final Map propertyDescriptorCache;


	/**
	 * Create a new CachedIntrospectionResults instance for the given class.
	 */
	private CachedIntrospectionResults2(Class clazz) throws BeansException {
		try {
			if (logger.isDebugEnabled()) {
				logger.debug("Getting BeanInfo for class [" + clazz.getName() + "]");
			}
			this.beanInfo = Introspector.getBeanInfo(clazz);

			// Immediately remove class from Introspector cache, to allow for proper
			// garbage collection on class loader shutdown - we cache it here anyway,
			// in a GC-friendly manner. In contrast to CachedIntrospectionResults,
			// Introspector does not use WeakReferences as values of its WeakHashMap!
			Class classToFlush = clazz;
			do {
				Introspector.flushFromCaches(classToFlush);
				classToFlush = classToFlush.getSuperclass();
			}
			while (classToFlush != null);

			if (logger.isDebugEnabled()) {
				logger.debug("Caching PropertyDescriptors for class [" + clazz.getName() + "]");
			}
			this.propertyDescriptorCache = new HashMap();

			// This call is slow so we do it once.
			PropertyDescriptor[] pds = this.beanInfo.getPropertyDescriptors();
			for (int i = 0; i < pds.length; i++) {
				PropertyDescriptor pd = pds[i];
				if (logger.isDebugEnabled()) {
					logger.debug("Found bean property '" + pd.getName() + "'" +
							(pd.getPropertyType() != null ?
							" of type [" + pd.getPropertyType().getName() + "]" : "") +
							(pd.getPropertyEditorClass() != null ?
							"; editor [" + pd.getPropertyEditorClass().getName() + "]" : ""));
				}
				this.propertyDescriptorCache.put(pd.getName(), pd);
			}
		}
		catch (IntrospectionException ex) {
			throw new FatalBeanException("Cannot get BeanInfo for object of class [" + clazz.getName() + "]", ex);
		}
	}

	public BeanInfo getBeanInfo() {
		return this.beanInfo;
	}

	public Class getBeanClass() {
		return this.beanInfo.getBeanDescriptor().getBeanClass();
	}

	public PropertyDescriptor getPropertyDescriptor(String propertyName) {
		return (PropertyDescriptor) this.propertyDescriptorCache.get(propertyName);
	}

}

Also I've proven (at least locally on my laptop) that solution works with test below.
To run test below you need to change some private members of CachedIntrospectionResults to public access.
Also you will need to give some jar with some class to test against it (here mysql jdbc driver had been used).

import java.beans.BeanInfo;
import java.io.File;
import java.lang.ref.Reference;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.Map;

import junit.framework.TestCase;

import org.springframework.beans.CachedIntrospectionResults;
import org.springframework.beans.CachedIntrospectionResults2;

public class CacheTest extends TestCase {

	public void testCache() throws Exception {
		File jar = new File("./jdbc-3.1.10.jar");
		URLClassLoader cl = URLClassLoader.newInstance(new URL[] { jar.toURL() });

		Class clazz = cl.loadClass("com.mysql.jdbc.Driver");

		{
			CachedIntrospectionResults cir = CachedIntrospectionResults.forClass(clazz);

			assertFalse(cir.isCacheSafe(clazz));

			assertTrue(CachedIntrospectionResults.classCache.containsKey(clazz));

			Class beanClass = cir.getBeanClass();

			assertEquals(clazz, beanClass);

			BeanInfo beanInfo = cir.getBeanInfo();

			assertNotNull(beanInfo);
		}

		{
			Reference ref = (Reference) CachedIntrospectionResults.classCache.get(clazz);
			assertNotNull(ref.get());
		}
		for (int i = 0; i < 100; i++) {

			System.gc();
			Thread.sleep(10);
			Reference ref = (Reference) CachedIntrospectionResults.classCache.get(clazz);
			if (ref.get() == null) {
				System.out.println("EVICTED on #" + i + " gc pass " + clazz);

				return;
			}

			fail("CachedIntrospectionResults should be evicted from cache.");
		}

	}

	public void testCache2() throws Exception {
		File jar = new File("./jdbc-3.1.10.jar");
		URLClassLoader cl = URLClassLoader.newInstance(new URL[] { jar.toURL() });

		Class clazz = cl.loadClass("com.mysql.jdbc.Driver");

		{
			CachedIntrospectionResults2 cir = CachedIntrospectionResults2.forClass(clazz);

			assertTrue(CachedIntrospectionResults2.classLoaderCache.containsKey(cl));
			assertTrue(((Map) CachedIntrospectionResults2.classLoaderCache.get(cl)).containsKey(clazz));

			Class beanClass = cir.getBeanClass();

			assertEquals(clazz, beanClass);

			BeanInfo beanInfo = cir.getBeanInfo();

			assertNotNull(beanInfo);
		}

		{
			CachedIntrospectionResults2 cir = (CachedIntrospectionResults2) ((Map) CachedIntrospectionResults2.classLoaderCache
					.get(cl)).get(clazz);
			assertNotNull(cir);
		}
		for (int i = 0; i < 100; i++) {
			System.out.println("P1 PASS #" + i);

			System.gc();
			Thread.sleep(10);
			CachedIntrospectionResults2 cir = (CachedIntrospectionResults2) ((Map) CachedIntrospectionResults2.classLoaderCache
					.get(cl)).get(clazz);
			if (cir == null) {
				System.out.println("EVICTED on gc pass #" + i + "  clazz :" + clazz);
				fail("Should not be evicted without evicting  classloader first.");

			}

		}

		assertEquals(1, CachedIntrospectionResults2.classLoaderCache.size());
		cl = null; // Releasing classloader
		clazz = null; // Releasing class
		for (int i = 0; i < 100; i++) {
			System.out.println("P2 PASS #" + i);
			System.gc();
			Thread.sleep(10);
			int size = CachedIntrospectionResults2.classLoaderCache.size();
			if (size == 0) {
				System.out.println("EVICTED gc pass on #" + i);
				return;

			}

		}

		fail("Should be evicted after unloading classloader.");

	}

Tested on jdk1.5.0_10 SUN Linux (Centos 5.0)

I plan to test this solution further on a staging environment for my current project and I will post results.

---

Affects: 1.0 RC1, 1.0 RC2, 1.0 final, 1.0.1, 1.0.2, 1.1 RC1, 1.1 RC2, 1.1 final, 1.1.1, 1.1.2, 1.1.3, 1.1.4, 1.1.5, 1.2 RC1, 1.2 RC2, 1.2 final, 1.2.1, 1.2.2, 1.2.3, 1.2.4, 1.2.5, 1.2.6, 1.2.7, 1.2.8, 2.0 M1, 2.0 M2, 2.0 M3, 2.0 M4, 2.0 M5, 2.0 RC1, 2.0 RC2, 2.0 RC3, 2.0 RC4, 2.0 final, 2.0.1, 2.0.2, 1.2.9, 2.0.3, 2.0.4, 2.0.5, 2.0.6, 2.0.7, 2.0.8, 2.1 M1, 2.1 M2, 2.1 M3, 2.1 M4, 2.5 RC1, 2.5 RC2, 2.5 final, 2.5.1, 2.5.2, 2.5.3, 2.5.4

Sub-tasks:

• SPR-4876 [Performance] CachedIntrospectionResults initialization slow down weblogic. [SPR-7838] #12494 SPR-4876 [Performance] CachedIntrospectionResults initialization slow down weblogic.

Issue Links:

• GC problem when having Spring jars in a higher ClassLoader without using clearClassLoader [SPR-7227] #11886 GC problem when having Spring jars in a higher ClassLoader without using clearClassLoader

Referenced from: commits 81e683b

2 votes, 5 watchers

[spring-projects-issues]

Tomasz Wysocki commented

I've made some further investigations on how things are handled and I found that:

1. BeanInfo interface is rarely implemented by explicit "BeanInfo" classes, what is used the most is reflection-based GenericBeanInfo.
2. GenericBeanInfo and all object it references, PropertyDescriptor, BeanDescriptor and MethodDescriptor do not hold any strong references to the class it reflects. All of mentioned classes are carefully crafted not to make any strong reference to class it describes, instead all are using soft/weak references. Unfortunately it is taken care of since 1.5.0 (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4809008)

Thus, even if we strongly reference GenericBeanInfo objects (since 1.5.0), we do not strongly reference classes and do not prohibit their unloading.
The only problem is when application does provide its own custom BeanInfo implementation which does not follow the rule not to make strong references to a class itself.
There is no simple way to check if BeanInfo has strong reference to a class, so one can revert to "isCacheSafe" implementation that checks if bean info class is instance of "GenericBeanInfo" and thrust that as being a good implementation. The problem is still unresolved in beans package - http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5102804.

All that is true in 1.5.0, 1.4 has serious problems that GenericBeanInfo will refer strongly to the reflected class and since Spring still supports 1.4, current approach (to have custom caching) has to stay.

We can improve by checking JDK version and treat JDK1.5+ with GenericBeanInfo as "cache-safe", regardless of classloader of a class. The patch above is obsolete.
Something like this can be employed:

public static boolean isCacheSafe(CachedIntrospectionResults results) {
if (JdkVersion.isAtLeastJava15() && results.getBeanInfo().getClass().getName().equals("java.beans.GenericBeanInfo")) {
return true;
}
.... rest of a method

Sorry for long considerations, hope I had proposed something useful from your point of view. My goal was to avoid application explicit registration/deregistration of its classloaders.

[spring-projects-issues]

Juergen Hoeller commented

You make a valid point there, and we will consider this for Spring 3.0 - which will require JDK 1.5 or higher anyway.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

As of Spring 3.0 M3, CachedIntrospectionResults always caches bean classes except in case of a custom BeanInfo class being provided in a non-safe ClassLoader.

Juergen