RFR: 8159746: (proxy) Support for default methods
Mandy Chung
mchung at openjdk.java.net
Fri Nov 20 19:55:09 UTC 2020
On Fri, 20 Nov 2020 19:11:54 GMT, Mandy Chung <mchung at 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
More information about the core-libs-dev
mailing list