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

[Peter Levart]

Hi Mandy,

On 04/08/2016 09:45 PM, Mandy Chung wrote:
>> 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.

In one of previous iterations, the abstract class was ClassLoaderValue 
and the two concrete subclasses were ClassLoaderValue.Root and 
ClassLoadeValue.Sub. In such arrangement, the types were called:

ClassLoaderValue.Root<V> clv = ....;
ClassLoaderValue.Root<V>.Sub<K1>.Sub<k2> clv_k12 = clv.sub(k1).sub(k2);

In webrev.05, I renamed abstract class to AbstractClassLoaderValue and 
the concrete subclasses to ClassLoaderValue (the root) and 
AbstractClassLoaderValue.Sub.

Sub is special, because it must be a subclass of abstract class and an 
inner class of abstract class at the same time, so it must be nested 
inside AbstractClassLoaderValue. Type names in this arrangement are 
nicer looking:

ClassLoaderValue<V> clv = ....;
ClassLoaderValue<V>.Sub<K1>.Sub<k2> clv_k12 = clv.sub(k1).sub(k2);

If Sub was nested inside concrete ClassLoaderValue, then we could 
compose an instance of ClassLoaderValue<V>.Sub<K>, but could not 
continue composition and compose an instance of 
ClassLoaderValue<V>.Sub<K1>.Sub<K2> for example.

> AbstractClassLoaderValue.java
>   263              ? BootLoader.getClassLoaderValueMap()
>   264              : JLA.createOrGetClassLoaderValueMap(cl)
>
> Nit: I generally like ?-ternary indent to the right below the test.

Reformatted.

> 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.

Added javadoc for getProxyConstructor and removed declared IAE.

>   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.

I would still rather keep it that way. For example if there is a race 
with a thread setting SecurityManager, then without this check, NPE 
could result.

>   532             return Boolean.TRUE.equals(
>   533                 reverseProxyCache.sub(c).get(c.getClassLoader()));
>
> Maybe Objects::equals that checks == first.

Ok.

> 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.

Moved.

>> @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.

Agreed.

So here's updated webrev:

http://cr.openjdk.java.net/~plevart/jdk9-dev/Proxy.caching/webrev.06/

> Mandy

Regards, Peter