> On Apr 8, 2016, at 3:41 AM, Peter Levart <peter.lev...@gmail.com> wrote: > > Hi, > > With Mandy we have prepared the following patch to restore performance of > java.lanf.reflect.Proxy class caching that has regressed with introduction of > additional checks that have to be performed due to modules: > > http://cr.openjdk.java.net/~plevart/jdk9-dev/Proxy.caching/webrev.05/ >
Thanks for taking this on and further improving the performance. With this patch, existing code could move to newProxyInstance and remove any code to cache Proxy classes for performance reason. The change looks quite good. Minor comments below. I like ClassLoaderValue idea that per-class loader proxy classes and dynamic modules are cached there that allows them to be GC’ed together. ClassLoaderValue might be useful for some other area and when those are identified, we could consider moving it to jdk.internal. The javadoc of ClassLoaderValue has references to root-ClassLoaderValue and sub-ClassLoaderValue. Sub is an inner class in AbstractClassLoaderValue. Should it be be ClassLoaderValue.Sub? They are minor points as this is for proxy use only. Something we could look into when we want to move it out from package scope. AbstractClassLoaderValue.java 263 ? BootLoader.getClassLoaderValueMap() 264 : JLA.createOrGetClassLoaderValueMap(cl) Nit: I generally like ?-ternary indent to the right below the test. Proxy.java 376 private static Constructor<?> getProxyConstructor(Class<?> caller, // null if no SecurityManager 377 ClassLoader loader, 378 Class<?>... interfaces) 379 throws IllegalArgumentException 380 { IAE is an unchecked exception that does not need to be declared. I realized it’s pre-existing code. While you are on it, it can be taken out. The informal javadoc for getProxyConstructor would be helpful. So the comment next to the caller parameter would be moved to the javadoc. 384 if (caller != null) { 385 checkProxyAccess(caller, loader, intf); 386 } The conditional check (caller != null) is not needed since checkProxyAccess and checkNewProxyPermission are nop if security manager is not present. Same also applies to line 986. 532 return Boolean.TRUE.equals( 533 reverseProxyCache.sub(c).get(c.getClassLoader())); Maybe Objects::equals that checks == first. Driver.java 28 * @bug 8152115 29 * @summary functional and concurrency test for ClassLoaderValue The convention we use to move @bug and @summary after @test. It’d be good to move them up. > > @Mandy > > I haven't yet removed the superfluous checking and providing access from > dynamic module to the modules/packages of transitive closure of interfaces > implemented by proxy class. I think this should be done in a separate issue > so that this change doesn't change the semantics of implementation. > I agree better to separate this as a different issue. Perhaps I can take that one since I’d like to do more testing before pushing it. Mandy