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 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; > > } > > > > } > > > > >
