RFR: JDK-8152115 (proxy) Examine performance of dynamic proxy creation

[Mandy Chung]

> On Apr 8, 2016, at 3:41 AM, Peter Levart <peter.levart at 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