RFR: 8159746: (proxy) Support for default methods

[Mandy Chung]

On Fri, 20 Nov 2020 11:18:32 GMT, Peter Levart <plevart at openjdk.org> wrote:

>> Hi Mandy,
>> I re-ran the benchmark on your latest version (the static API) and I get similar results as you:
>> 
>> Benchmark               Mode  Cnt   Score   Error  Units
>> ProxyBench.implClass    avgt    5   3.745 ± 0.033  ns/op
>> ProxyBench.implProxy    avgt    5  29.826 ± 0.183  ns/op
>> ProxyBench.ppImplClass  avgt    5   3.683 ± 0.009  ns/op
>> ProxyBench.ppImplProxy  avgt    5  29.124 ± 0.535  ns/op
>> 
>> I also tried a variant where the access check in the invokeDefault static method is not pre-screened with checking of the interface modifiers and package export status but relies on Method.checkAccess cache alone:
>> 
>>         // access check
>>         Class<?> caller = Reflection.getCallerClass();
>>         int modifiers = method.getModifiers();
>>         method.checkAccess(caller, intf, proxyClass, modifiers);
>> 
>> ... and the results are not worse, even marginally better:
>> 
>> Benchmark               Mode  Cnt   Score   Error  Units
>> ProxyBench.implClass    avgt    5   3.724 ± 0.012  ns/op
>> ProxyBench.implProxy    avgt    5  29.138 ± 0.271  ns/op
>> ProxyBench.ppImplClass  avgt    5   3.744 ± 0.009  ns/op
>> ProxyBench.ppImplProxy  avgt    5  28.961 ± 0.182  ns/op
>> 
>> I think this looks reasonably good.
>
> 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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/313