On Fri, 20 Nov 2020 11:18:32 GMT, Peter Levart <plev...@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
