> On Apr 8, 2016, at 3:41 AM, Peter Levart <[email protected]> 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
