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