Freeman, This looks much better. Can you get this merged in? Thanks!
Dan > On Dec 24, 2018, at 4:50 AM, Freeman Fang <[email protected]> wrote: > > Hi Dan, > > I just revised to use ClassValue as ProxyClassLoaderCache backend, and I > pushed the changes to ProxyClassLoaderCache branch, would you please take a > look? > > Merry Christmas! > －－－－－－－－－－－－－ > Freeman(Yue) Fang > > Red Hat, Inc. > > > > > >> On Dec 21, 2018, at 11:43 PM, Daniel Kulp <[email protected] >> <mailto:[email protected]>> wrote: >> >> This commit has a bunch of problems. >> >> At no point is anything removed from these caches until the cache fills. >> Thus, would definitely be considered a memory leak of some sort. >> >> Also, because the class loader is held onto strongly, the classes are locked >> and not able to be garbage collected. This would prevent apps from being >> unloaded, result in locked jar/wars/ears, etc… >> >> This really needs to be reverted before 3.3. >> >> My suggestion, I think, would be if you can determine which interface in the >> array is the user one (and not CXF/JDK), use a ClassValue<ClassLoader> to >> compute and hold this. >> >> >> >> Dan >> >> >> >>> On Dec 21, 2018, at 12:32 AM, [email protected] <mailto:[email protected]> >>> wrote: >>> >>> This is an automated email from the ASF dual-hosted git repository. >>> >>> ffang pushed a commit to branch master >>> in repository https://gitbox.apache.org/repos/asf/cxf.git >>> <https://gitbox.apache.org/repos/asf/cxf.git> >>> >>> >>> The following commit(s) were added to refs/heads/master by this push: >>> new af33b2a [CXF-7934]Add cache for ProxyClassLoader >>> af33b2a is described below >>> >>> commit af33b2a28c39504388ea370f6849a06d551b4dd1 >>> Author: Freeman Fang <[email protected] >>> <mailto:[email protected]>> >>> AuthorDate: Fri Dec 21 13:32:19 2018 +0800 >>> >>> [CXF-7934]Add cache for ProxyClassLoader >>> --- >>> .../org/apache/cxf/common/util/ProxyHelper.java | 53 >>> +++++++++++++++++++++- >>> .../org/apache/cxf/jaxrs/utils/InjectionUtils.java | 50 +++++++++++++++++--- >>> 2 files changed, 95 insertions(+), 8 deletions(-) >>> >>> diff --git a/core/src/main/java/org/apache/cxf/common/util/ProxyHelper.java >>> b/core/src/main/java/org/apache/cxf/common/util/ProxyHelper.java >>> index 7b185aa..ad83ad9 100644 >>> --- a/core/src/main/java/org/apache/cxf/common/util/ProxyHelper.java >>> +++ b/core/src/main/java/org/apache/cxf/common/util/ProxyHelper.java >>> @@ -24,6 +24,13 @@ import java.lang.reflect.Method; >>> import java.lang.reflect.Proxy; >>> import java.security.AccessController; >>> import java.security.PrivilegedAction; >>> +import java.util.Collections; >>> +import java.util.HashMap; >>> +import java.util.Map; >>> +import java.util.logging.Level; >>> +import java.util.logging.Logger; >>> + >>> +import org.apache.cxf.common.logging.LogUtils; >>> >>> /** >>> * >>> @@ -39,8 +46,15 @@ public class ProxyHelper { >>> } >>> HELPER = theHelper; >>> } >>> - >>> - >>> + >>> + private static final Logger LOG = >>> LogUtils.getL7dLogger(ProxyHelper.class); >>> + >>> + protected Map<String, ClassLoader> proxyClassLoaderCache = >>> + Collections.synchronizedMap(new HashMap<String, ClassLoader>()); >>> + protected int cacheSize = >>> + >>> Integer.parseInt(System.getProperty("org.apache.cxf.proxy.classloader.size", >>> "3000")); >>> + >>> + >>> protected ProxyHelper() { >>> } >>> >>> @@ -59,9 +73,26 @@ public class ProxyHelper { >>> */ >>> private ClassLoader getClassLoaderForInterfaces(final ClassLoader >>> loader, final Class<?>[] interfaces) { >>> if (canSeeAllInterfaces(loader, interfaces)) { >>> + LOG.log(Level.FINE, "current classloader " + loader + " can >>> see all interface"); >>> return loader; >>> } >>> + String sortedNameFromInterfaceArray = >>> getSortedNameFromInterfaceArray(interfaces); >>> + ClassLoader cachedLoader = >>> proxyClassLoaderCache.get(sortedNameFromInterfaceArray); >>> + if (cachedLoader != null) { >>> + if (canSeeAllInterfaces(cachedLoader, interfaces)) { >>> + //found cached loader >>> + LOG.log(Level.FINE, "find required loader from >>> ProxyClassLoader cache with key" >>> + + sortedNameFromInterfaceArray); >>> + return cachedLoader; >>> + } else { >>> + //found cached loader somehow can't see all interfaces >>> + LOG.log(Level.FINE, "find a loader from ProxyClassLoader >>> cache with key " >>> + + sortedNameFromInterfaceArray >>> + + " but can't see all interfaces"); >>> + } >>> + } >>> ProxyClassLoader combined; >>> + LOG.log(Level.FINE, "can't find required ProxyClassLoader from >>> cache, create a new one with parent " + loader); >>> final SecurityManager sm = System.getSecurityManager(); >>> if (sm == null) { >>> combined = new ProxyClassLoader(loader, interfaces); >>> @@ -75,9 +106,26 @@ public class ProxyHelper { >>> } >>> for (Class<?> currentInterface : interfaces) { >>> combined.addLoader(getClassLoader(currentInterface)); >>> + LOG.log(Level.FINE, "interface for new created >>> ProxyClassLoader is " >>> + + currentInterface.getName()); >>> + LOG.log(Level.FINE, "interface's classloader for new created >>> ProxyClassLoader is " >>> + + currentInterface.getClassLoader()); >>> } >>> + if (proxyClassLoaderCache.size() >= cacheSize) { >>> + LOG.log(Level.FINE, "proxyClassLoaderCache is full, need clear >>> it"); >>> + proxyClassLoaderCache.clear(); >>> + } >>> + proxyClassLoaderCache.put(sortedNameFromInterfaceArray, combined); >>> return combined; >>> } >>> + >>> + private String getSortedNameFromInterfaceArray(Class<?>[] interfaces) { >>> + SortedArraySet<String> arraySet = new SortedArraySet<String>(); >>> + for (Class<?> currentInterface : interfaces) { >>> + arraySet.add(currentInterface.getName() + >>> currentInterface.getClassLoader()); >>> + } >>> + return arraySet.toString(); >>> + } >>> >>> private static ClassLoader getClassLoader(final Class<?> clazz) { >>> final SecurityManager sm = System.getSecurityManager(); >>> @@ -123,4 +171,5 @@ public class ProxyHelper { >>> public static Object getProxy(ClassLoader loader, Class<?>[] interfaces, >>> InvocationHandler handler) { >>> return HELPER.getProxyInternal(loader, interfaces, handler); >>> } >>> + >>> } >>> diff --git >>> a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/InjectionUtils.java >>> >>> b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/InjectionUtils.java >>> index ce4d239..e2c80e1 100644 >>> --- >>> a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/InjectionUtils.java >>> +++ >>> b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/InjectionUtils.java >>> @@ -137,7 +137,13 @@ public final class InjectionUtils { >>> private static final String ENUM_CONVERSION_CASE_SENSITIVE = >>> "enum.conversion.case.sensitive"; >>> >>> private static final String IGNORE_MATRIX_PARAMETERS = >>> "ignore.matrix.parameters"; >>> - >>> + >>> + private static Map<String, ProxyClassLoader> proxyClassLoaderCache = >>> + Collections.synchronizedMap(new HashMap<String, >>> ProxyClassLoader>()); >>> + >>> + private static int cacheSize = >>> + >>> Integer.parseInt(System.getProperty("org.apache.cxf.proxy.classloader.size", >>> "3000")); >>> + >>> private InjectionUtils() { >>> >>> } >>> @@ -1076,9 +1082,25 @@ public final class InjectionUtils { >>> proxy = createThreadLocalServletApiContext(type.getName()); >>> } >>> if (proxy == null) { >>> - ProxyClassLoader loader = new >>> ProxyClassLoader(Proxy.class.getClassLoader()); >>> - loader.addLoader(type.getClassLoader()); >>> - loader.addLoader(ThreadLocalProxy.class.getClassLoader()); >>> + ProxyClassLoader loader >>> + = proxyClassLoaderCache.get(type.getName() + >>> type.getClassLoader()); >>> + if (loader == null >>> + || !canSeeAllClasses(loader, new Class<?>[]{Proxy.class, >>> type, ThreadLocalProxy.class})) { >>> + // to avoid creating too much ProxyClassLoader to save >>> Metaspace usage >>> + LOG.log(Level.FINE, "can't find required ProxyClassLoader >>> for type " + type.getName()); >>> + LOG.log(Level.FINE, "create a new one with parent " + >>> Proxy.class.getClassLoader()); >>> + loader = new >>> ProxyClassLoader(Proxy.class.getClassLoader()); >>> + loader.addLoader(type.getClassLoader()); >>> + LOG.log(Level.FINE, "type classloader is " + >>> type.getClassLoader()); >>> + loader.addLoader(ThreadLocalProxy.class.getClassLoader()); >>> + LOG.log(Level.FINE, "ThreadLocalProxy classloader is " >>> + + >>> ThreadLocalProxy.class.getClassLoader().getClass().getName()); >>> + if (proxyClassLoaderCache.size() >= cacheSize) { >>> + LOG.log(Level.FINE, "proxyClassLoaderCache is full, >>> need clear it"); >>> + proxyClassLoaderCache.clear(); >>> + } >>> + proxyClassLoaderCache.put(type.getName() + >>> type.getClassLoader(), loader); >>> + } >>> return (ThreadLocalProxy<T>)Proxy.newProxyInstance(loader, >>> new Class[] {type, ThreadLocalProxy.class >>> }, >>> new ThreadLocalInvocationHandler<T>()); >>> @@ -1086,8 +1108,24 @@ public final class InjectionUtils { >>> >>> return (ThreadLocalProxy<T>)proxy; >>> } >>> - >>> - private static boolean isServletApiContext(String name) { >>> + >>> + private static boolean canSeeAllClasses(ClassLoader loader, Class<?>[] >>> interfaces) { >>> + for (Class<?> currentInterface : interfaces) { >>> + String ifName = currentInterface.getName(); >>> + try { >>> + Class<?> ifClass = Class.forName(ifName, true, loader); >>> + if (ifClass != currentInterface) { >>> + return false; >>> + } >>> + >>> + } catch (NoClassDefFoundError | ClassNotFoundException e) { >>> + return false; >>> + } >>> + } >>> + return true; >>> + } >>> + >>> + private static boolean isServletApiContext(String name) { >>> return name.startsWith("javax.servlet."); >>> } >>> >>> >> >> -- >> Daniel Kulp >> [email protected] <mailto:[email protected]> <mailto:[email protected] >> <mailto:[email protected]>> - http://dankulp.com/blog >> <http://dankulp.com/blog> <http://dankulp.com/blog <http://dankulp.com/blog>> >> Talend Community Coder - http://talend.com <http://talend.com/> >> <http://coders.talend.com/ <http://coders.talend.com/>> -- Daniel Kulp [email protected] <mailto:[email protected]> - http://dankulp.com/blog <http://dankulp.com/blog> Talend Community Coder - http://talend.com <http://coders.talend.com/>
