On Fri, 20 Nov 2020 19:11:54 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> In my previous attempts when I was modifying the ProxyGenerator I noticed
>> that generated proxy fields holding Method objects are just "static" but
>> could be "static final". If they were "static final", JIT could
>> constant-fold the Method instances. If this was combined with marking some
>> fields in Method as @Stable, this could improve optimization of code a bit
>> further. I did this with the following patch:
>>
>> Index: src/java.base/share/classes/java/lang/reflect/InvocationHandler.java
>> IDEA additional info:
>> Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
>> <+>UTF-8
>> ===================================================================
>> --- src/java.base/share/classes/java/lang/reflect/InvocationHandler.java
>> (revision 8352a20bf54a085a97d3f703b5dab482d3f9eccc)
>> +++ src/java.base/share/classes/java/lang/reflect/InvocationHandler.java
>> (date 1605869089804)
>> @@ -271,15 +271,10 @@
>> throw new IllegalArgumentException(""" + method + "" is not a
>> default method");
>> }
>> Class<?> intf = method.getDeclaringClass();
>> - // access check if it is a non-public proxy interface or not
>> unconditionally exported
>> - if (!Modifier.isPublic(intf.getModifiers()) ||
>> - !intf.getModule().isExported(intf.getPackageName())) {
>> - // throw IAE if the caller class has no access to the default
>> method
>> - // same access check to Method::invoke on the default method
>> - int modifiers = method.getModifiers();
>> - Class<?> caller = Reflection.getCallerClass();
>> - method.checkAccess(caller, intf, proxyClass, modifiers);
>> - }
>> + // access check
>> + Class<?> caller = Reflection.getCallerClass();
>> + int modifiers = method.getModifiers();
>> + method.checkAccess(caller, intf, proxyClass, modifiers);
>>
>> MethodHandle mh = Proxy.defaultMethodHandle(proxyClass, method);
>> // invoke the super method
>> Index: src/java.base/share/classes/java/lang/reflect/Method.java
>> IDEA additional info:
>> Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
>> <+>UTF-8
>> ===================================================================
>> --- src/java.base/share/classes/java/lang/reflect/Method.java
>> (revision 8352a20bf54a085a97d3f703b5dab482d3f9eccc)
>> +++ src/java.base/share/classes/java/lang/reflect/Method.java (date
>> 1605870445135)
>> @@ -31,6 +31,7 @@
>> import jdk.internal.reflect.Reflection;
>> import jdk.internal.vm.annotation.ForceInline;
>> import jdk.internal.vm.annotation.IntrinsicCandidate;
>> +import jdk.internal.vm.annotation.Stable;
>> import sun.reflect.annotation.ExceptionProxy;
>> import sun.reflect.annotation.TypeNotPresentExceptionProxy;
>> import sun.reflect.generics.repository.MethodRepository;
>> @@ -66,7 +67,7 @@
>> * @since 1.1
>> */
>> public final class Method extends Executable {
>> - private Class<?> clazz;
>> + @Stable private Class<?> clazz;
>> private int slot;
>> // This is guaranteed to be interned by the VM in the 1.4
>> // reflection implementation
>> @@ -74,7 +75,7 @@
>> private Class<?> returnType;
>> private Class<?>[] parameterTypes;
>> private Class<?>[] exceptionTypes;
>> - private int modifiers;
>> + @Stable private int modifiers;
>> // Generics and annotations support
>> private transient String signature;
>> // generic info repository; lazily initialized
>> Index: src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
>> IDEA additional info:
>> Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
>> <+>UTF-8
>> ===================================================================
>> --- src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
>> (revision 8352a20bf54a085a97d3f703b5dab482d3f9eccc)
>> +++ src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
>> (date 1605870027579)
>> @@ -490,7 +490,7 @@
>> for (List<ProxyMethod> sigmethods : proxyMethods.values()) {
>> for (ProxyMethod pm : sigmethods) {
>> // add static field for the Method object
>> - visitField(Modifier.PRIVATE | Modifier.STATIC,
>> pm.methodFieldName,
>> + visitField(Modifier.PRIVATE | Modifier.STATIC |
>> Modifier.FINAL, pm.methodFieldName,
>> LJLR_METHOD, null, null);
>>
>> // Generate code for proxy method
>>
>> ...and this makes the following results:
>>
>> Benchmark Mode Cnt Score Error Units
>> ProxyBench.implClass avgt 5 3.766 ± 0.040 ns/op
>> ProxyBench.implProxy avgt 5 26.847 ± 0.626 ns/op
>> ProxyBench.ppImplClass avgt 5 3.700 ± 0.017 ns/op
>> ProxyBench.ppImplProxy avgt 5 26.322 ± 0.048 ns/op
>>
>> But this can be changed as a follow-up patch that also takes care of
>> Constructor and Field.
>
> @plevart I'm okay with this slight performance improvement.
I have a fresh look at the different options we have explored (lots of
challenges in finding the right API with security, usability and performance
issues to consider). I agree with Remi that we should keep the design and API
simple and makes it easier to invoke default methods with today's Proxy API.
We can design a better Proxy API in the future.
The options we have explored are:
1. static `InvocationHandler::invokeDefault` method
2. abstract `DelegatingInvocationHandler` class with a protected
`invokeDefault` method
3. a new `newProxyInstance` factory method taking a function that produces an
invocation handler with the ability to invoke a default method via a
`superHandler`
(1) is very simple API but caller-sensitive. No other API change. Access
check done at default method invocation time (which is consistent with the core
reflection `Method::invoke`). It shares the caller class caching in
`Method::checkAccess` which helps the performance. The performance overhead
is slightly higher than (2) & (3) which does access check at proxy creation
time.
(2) is simple and I like that the `invokeDefault` can be enforced to be invoked
only by the proxy's invocation handler. However this requires more API
changes (including `newProxyInstance`, `getInvocationHandler`, and new
unchecked exception type). (3) is clever but a bit over-rotated (how Alan
describes it) to allow it to be expressed in a lambda expression. If an
invocation handler wants to save `superHandler`, it can't assign it to a final
field in lambda and would need to workaround it writing to an array element.
I will go with option (1) - static `invokeDefault` method [1] unless there is
any objection.
Here is the specdiff:
http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/specdiff/
[1]
http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/api/java.base/java/lang/reflect/InvocationHandler.html#invokeDefault(java.lang.Object,java.lang.reflect.Method,java.lang.Object...)
-------------
PR: https://git.openjdk.java.net/jdk/pull/313
