Hi Dan, On Fri, Apr 24, 2015 at 3:13 PM, Dan Haywood <[email protected]> wrote:
> Hi Martin, > > thx for the input. I did consider this, but realized that I don't know > about the interaction of class(un)loading vs GC. > > My understanding (correct me if I'm wrong) is that GC daemon will quite > eagerly clean up any objects if there are only weak references to that > object. So I was worried about our ProxyFactory instances - that we want > to hold on to for as long as there are any instances of the class being > enhanced - might get GC'd away. > > But thinking a bit deeper, I suspect that the above sentence is nonsense... > if we have weak keys for the Class', then of course the container itself > will have strong references to those classes. And, as you say, only if the > Right. The ClassLoader that loaded the Class has a reference to it, so the Class will be eligible for GC only if the ClassLoader is unloaded/GC-ed. The ClassLoader could not be unloaded if its Class-es are strong referenced by something else. This is what I meant by "Class Loader leak". By using weak key you say "I'm fine to remove this entry if the key is not referenced anywhere else. Additionally by using "-XX:+CMSClassUnloadingEnabled" (with -XX:+UseConcMarkSweepGC) you can hint the JVM to unload dynamically created classes which ClassLoaders are still around. > container tries to unload the Class then the map won't be "strong enough" > to hold on. > > Correct me if I'm wrong; but if that's what you were thinking, then I > agree. > > I'll make the change. > > Thx again > Dan > > > > On 24 April 2015 at 13:00, Martin Grigorov <[email protected]> wrote: > > > On Fri, Apr 24, 2015 at 1:15 PM, <[email protected]> wrote: > > > > > Repository: isis > > > Updated Branches: > > > refs/heads/master 69d559486 -> ee834d9a3 > > > > > > > > > ISIS-1137: Now cache a ProxyFactory instance by class (allowing reuse > of > > > the enhanced class for all instances of that type) rather than > creating a > > > newly enhanced class as currently. > > > > > > > > > Project: http://git-wip-us.apache.org/repos/asf/isis/repo > > > Commit: http://git-wip-us.apache.org/repos/asf/isis/commit/a4f924b1 > > > Tree: http://git-wip-us.apache.org/repos/asf/isis/tree/a4f924b1 > > > Diff: http://git-wip-us.apache.org/repos/asf/isis/diff/a4f924b1 > > > > > > Branch: refs/heads/master > > > Commit: a4f924b11cf614535e2faf2fb4f7aacdb4c7e1e0 > > > Parents: 69d5594 > > > Author: Dan Haywood <[email protected]> > > > Authored: Fri Apr 24 10:29:16 2015 +0100 > > > Committer: Dan Haywood <[email protected]> > > > Committed: Fri Apr 24 10:29:16 2015 +0100 > > > > > > ---------------------------------------------------------------------- > > > .../isis/core/commons/lang/ArrayExtensions.java | 4 +- > > > .../core/wrapper/WrapperFactoryAbstract.java | 6 +- > > > .../proxy/ProxyInstantiatorForJavassist.java | 71 > > +++++++++++++++----- > > > 3 files changed, 61 insertions(+), 20 deletions(-) > > > ---------------------------------------------------------------------- > > > > > > > > > > > > > > > http://git-wip-us.apache.org/repos/asf/isis/blob/a4f924b1/core/metamodel/src/main/java/org/apache/isis/core/commons/lang/ArrayExtensions.java > > > ---------------------------------------------------------------------- > > > diff --git > > > > > > a/core/metamodel/src/main/java/org/apache/isis/core/commons/lang/ArrayExtensions.java > > > > > > b/core/metamodel/src/main/java/org/apache/isis/core/commons/lang/ArrayExtensions.java > > > index 25a6f88..df7238b 100644 > > > --- > > > > > > a/core/metamodel/src/main/java/org/apache/isis/core/commons/lang/ArrayExtensions.java > > > +++ > > > > > > b/core/metamodel/src/main/java/org/apache/isis/core/commons/lang/ArrayExtensions.java > > > @@ -27,6 +27,8 @@ import java.util.Arrays; > > > import java.util.Collection; > > > import java.util.List; > > > > > > +import com.google.common.collect.Lists; > > > + > > > import org.apache.isis.core.commons.exceptions.IsisException; > > > > > > public final class ArrayExtensions { > > > @@ -71,7 +73,7 @@ public final class ArrayExtensions { > > > } > > > > > > public static <T> T[] combine(final T[]... arrays) { > > > - final List<T> combinedList = new ArrayList<T>(); > > > + final List<T> combinedList = Lists.newArrayList(); > > > for (final T[] array : arrays) { > > > for (final T t : array) { > > > combinedList.add(t); > > > > > > > > > > > > http://git-wip-us.apache.org/repos/asf/isis/blob/a4f924b1/core/wrapper/src/main/java/org/apache/isis/core/wrapper/WrapperFactoryAbstract.java > > > ---------------------------------------------------------------------- > > > diff --git > > > > > > a/core/wrapper/src/main/java/org/apache/isis/core/wrapper/WrapperFactoryAbstract.java > > > > > > b/core/wrapper/src/main/java/org/apache/isis/core/wrapper/WrapperFactoryAbstract.java > > > index ceea4a5..2d251ed 100644 > > > --- > > > > > > a/core/wrapper/src/main/java/org/apache/isis/core/wrapper/WrapperFactoryAbstract.java > > > +++ > > > > > > b/core/wrapper/src/main/java/org/apache/isis/core/wrapper/WrapperFactoryAbstract.java > > > @@ -51,11 +51,11 @@ public abstract class WrapperFactoryAbstract > > > implements WrapperFactory, Authenti > > > private AdapterManager adapterManager; > > > private ObjectPersistor objectPersistor; > > > > > > - private final ProxyContextHandler proxy; > > > + private final ProxyContextHandler proxyContextHandler; > > > > > > public WrapperFactoryAbstract(final ProxyInstantiator > > > proxyInstantiator) { > > > > > > - proxy = new ProxyContextHandler(proxyInstantiator); > > > + proxyContextHandler = new > > ProxyContextHandler(proxyInstantiator); > > > > > > dispatchersByEventClass.put(ObjectTitleEvent.class, new > > > InteractionEventDispatcherTypeSafe<ObjectTitleEvent>() { > > > @Override > > > @@ -221,7 +221,7 @@ public abstract class WrapperFactoryAbstract > > > implements WrapperFactory, Authenti > > > } > > > > > > protected <T> T createProxy(final T domainObject, final > > ExecutionMode > > > mode) { > > > - return proxy.proxy(domainObject, this, mode, > > > getAuthenticationSessionProvider(), getSpecificationLookup(), > > > getAdapterManager(), getObjectPersistor()); > > > + return proxyContextHandler.proxy(domainObject, this, mode, > > > getAuthenticationSessionProvider(), getSpecificationLookup(), > > > getAdapterManager(), getObjectPersistor()); > > > } > > > > > > @Override > > > > > > > > > > > > http://git-wip-us.apache.org/repos/asf/isis/blob/a4f924b1/core/wrapper/src/main/java/org/apache/isis/core/wrapper/proxy/ProxyInstantiatorForJavassist.java > > > ---------------------------------------------------------------------- > > > diff --git > > > > > > a/core/wrapper/src/main/java/org/apache/isis/core/wrapper/proxy/ProxyInstantiatorForJavassist.java > > > > > > b/core/wrapper/src/main/java/org/apache/isis/core/wrapper/proxy/ProxyInstantiatorForJavassist.java > > > index 3d2b838..cc0ee19 100644 > > > --- > > > > > > a/core/wrapper/src/main/java/org/apache/isis/core/wrapper/proxy/ProxyInstantiatorForJavassist.java > > > +++ > > > > > > b/core/wrapper/src/main/java/org/apache/isis/core/wrapper/proxy/ProxyInstantiatorForJavassist.java > > > @@ -19,21 +19,38 @@ > > > > > > package org.apache.isis.core.wrapper.proxy; > > > > > > -import java.lang.reflect.InvocationHandler; > > > import java.lang.reflect.Method; > > > +import java.util.Map; > > > > > > -import javassist.util.proxy.MethodFilter; > > > -import javassist.util.proxy.MethodHandler; > > > -import javassist.util.proxy.ProxyFactory; > > > -import javassist.util.proxy.ProxyObject; > > > +import com.google.common.collect.MapMaker; > > > > > > import org.apache.isis.applib.services.wrapper.WrapperObject; > > > +import org.apache.isis.applib.services.wrapper.WrappingObject; > > > import org.apache.isis.core.commons.lang.ArrayExtensions; > > > +import > > > > > > org.apache.isis.core.metamodel.specloader.classsubstitutor.JavassistEnhanced; > > > import > > org.apache.isis.core.wrapper.handlers.DelegatingInvocationHandler; > > > import org.apache.isis.core.wrapper.internal.util.Util; > > > > > > +import javassist.util.proxy.MethodFilter; > > > +import javassist.util.proxy.MethodHandler; > > > +import javassist.util.proxy.Proxy; > > > +import javassist.util.proxy.ProxyFactory; > > > + > > > public class ProxyInstantiatorForJavassist implements > ProxyInstantiator > > { > > > > > > + /** > > > + * Lazily constructed cache. > > > + */ > > > + private final Map<Class, ProxyFactory> proxyFactoryByClass; > > > + > > > > > > > > > > > > + public ProxyInstantiatorForJavassist() { > > > + this(new MapMaker().concurrencyLevel(10).<Class, > > > ProxyFactory>makeMap()); > > > > > > > I think this should use weak keys to prevent classloader leaks and allow > > class unloading. > > > > > > > + } > > > + > > > + public ProxyInstantiatorForJavassist(Map<Class, ProxyFactory> > > > proxyFactoryByClass) { > > > + this.proxyFactoryByClass = proxyFactoryByClass; > > > + } > > > + > > > @SuppressWarnings("unchecked") > > > public <T> T instantiateProxy(final DelegatingInvocationHandler<T> > > > handler) { > > > > > > @@ -44,24 +61,40 @@ public class ProxyInstantiatorForJavassist > implements > > > ProxyInstantiator { > > > if (clazz.isInterface()) { > > > return Util.createInstance(clazz, handler, > > > WrapperObject.class); > > > } else { > > > - final Class<T> enhancedClass = createEnhancedClass(clazz, > > > handler, WrapperObject.class); > > > - ProxyObject proxyObject = (ProxyObject) > > > Util.createInstance(enhancedClass); > > > - proxyObject.setHandler(new MethodHandler() { > > > + final ProxyFactory proxyFactory = proxyFactoryFor(clazz); > > > + > > > + final Class<T> enhancedClass = proxyFactory.createClass(); > > > + final Proxy proxy = (Proxy) > > > Util.createInstance(enhancedClass); > > > + > > > + proxy.setHandler(new MethodHandler() { > > > @Override > > > public Object invoke(Object self, Method thisMethod, > > > Method proceed, Object[] args) throws Throwable { > > > return handler.invoke(self, thisMethod, args); > > > } > > > }); > > > - return (T) proxyObject; > > > + > > > + return (T) proxy; > > > } > > > } > > > - > > > - @SuppressWarnings("unchecked") > > > - private static <T> Class<T> createEnhancedClass(final Class<T> > > > toProxyClass, final InvocationHandler handler, final Class<?>... > > > auxiliaryTypes) { > > > - > > > + > > > + private <T> ProxyFactory proxyFactoryFor(final Class<T> > > toProxyClass) > > > { > > > + ProxyFactory proxyFactory = > > proxyFactoryByClass.get(toProxyClass); > > > + if(proxyFactory == null) { > > > + proxyFactory = createProxyFactoryFor(toProxyClass); > > > + proxyFactoryByClass.put(toProxyClass, proxyFactory); > > > + } > > > + return proxyFactory; > > > + } > > > + > > > + private <T> ProxyFactory createProxyFactoryFor(final Class<T> > > > toProxyClass) { > > > + > > > final ProxyFactory proxyFactory = new ProxyFactory(); > > > + > > > proxyFactory.setSuperclass(toProxyClass); > > > - > > > > > > proxyFactory.setInterfaces(ArrayExtensions.combine(toProxyClass.getInterfaces(), > > > new > > > > > > Class<?>[]{org.apache.isis.core.metamodel.specloader.classsubstitutor.JavassistEnhanced.class}, > > > auxiliaryTypes)); > > > + final Class[] types = ArrayExtensions.combine( > > > + toProxyClass.getInterfaces(), > > > + new Class<?>[] { JavassistEnhanced.class, > > > WrappingObject.class }); > > > + proxyFactory.setInterfaces(types); > > > > > > proxyFactory.setFilter(new MethodFilter() { > > > @Override > > > @@ -70,8 +103,14 @@ public class ProxyInstantiatorForJavassist > implements > > > ProxyInstantiator { > > > return !m.getName().equals("finalize") || > m.isBridge(); > > > } > > > }); > > > - > > > - return proxyFactory.createClass(); > > > + > > > + // this is the default, I believe > > > + // calling it here only because I know that it will throw an > > > exception if the code were > > > + // in the future changed such that caching is invalid > > > + // (ie fail fast if future change could introduce a bug) > > > + proxyFactory.setUseCache(true); > > > + > > > + return proxyFactory; > > > } > > > > > > } > > > > > > > > >
