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