RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle
This reimplements core reflection with method handles. For `Constructor::newInstance` and `Method::invoke`, the new implementation uses `MethodHandle`. For `Field` accessor, the new implementation uses `VarHandle`.For the first few invocations of one of these reflective methods on a specific reflective object we invoke the corresponding method handle directly. After that we spin a dynamic bytecode stub defined in a hidden class which loads the target `MethodHandle` or `VarHandle` from its class data as a dynamically computed constant. Loading the method handle from a constant allows JIT to inline the method-handle invocation in order to achieve good performance. The VM's native reflection methods are needed during early startup, before the method-handle mechanism is initialized. That happens soon after System::initPhase1 and before System::initPhase2, after which we switch to using method handles exclusively. The core reflection and method handle implementation are updated to handle chained caller-sensitive method calls [1] properly. A caller-sensitive method can define with a caller-sensitive adapter method that will take an additional caller class parameter and the adapter method will be annotated with `@CallerSensitiveAdapter` for better auditing. See the detailed description from [2]. Ran tier1-tier8 tests. [1] https://bugs.openjdk.java.net/browse/JDK-8013527 [2] https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 - Commit messages: - fix whitespace - Merge branch 'master' of https://github.com/openjdk/jdk into reimplement-method-invoke - Merge branch 'master' of https://github.com/openjdk/jdk into reimplement-method-invoke - Merge - clean up test - Merge branch 'master' of https://github.com/openjdk/jdk into reimplement-method-invoke - cleanup - Merge branch 'master' of https://github.com/openjdk/jdk into reimplement-method-invoke - fix ClassByteBuilder to handle short field type properly. Support volatile fields - minor cleanup - ... and 5 more: https://git.openjdk.java.net/jdk/compare/cb368802...c575c3db Changes: https://git.openjdk.java.net/jdk/pull/5027/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271820 Stats: 7572 lines in 75 files changed: 7103 ins; 288 del; 181 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v2]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: minor test cleanup - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/c575c3db..b73a6faf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=00-01 Stats: 7 lines in 2 files changed: 0 ins; 6 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v2]
On Sat, 7 Aug 2021 02:07:24 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor test cleanup Hi Mandy, Quite a big change this has become. Good work! These are just preliminary observations. I'll try to do a more in-depth check in a week since I'm off on vacation in the coming week. I noticed there's a lot of complication due to logic that specializes the cases for small number of parameters via indirection through MHMethodAccessorDelegate just to avoid spreading the arguments of an Object[] array via the MH combinator asSpreader. Does this really bring noticeable performance improvement for the small number of arguments? I'm asking because the arguments in reflection API are already boxed and packed into an Object[] array and my feeling tells me that spreading them via MH combinator should have similar performance as doing that manually in the switch of DirectMethodAccessorImpl.XXX#invokeImpl methods. I may be wrong. If MH combinator is noticeably slower, I think it should be fixed. Another brittle part in my opinion is the AccessorUtils.isIllegalArgument method and the use of it in logic to determine where the caught exception comes from to wrap it in the correct type of exception for the reflection API. The logic of that method is based on assumptions of how a lot of other code is structured. Any change to the structure of that code can brake that logic. I liked the solution used in previous iterations where an exception handler was inserted in the MH chain immediately after the DMH which wrapped any Throwable into InvocationTargetException. So any naked exceptions thrown from MH chain could be contributed to argument processing. Why did you choose another route? Regards, Peter - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v2]
On Sat, 7 Aug 2021 02:07:24 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor test cleanup Hi Peter, I also like the previous simpler version as much as you do. However, both of these complications are done for the performance that I worked with @cl4es on these changes. Claes can explain and provide additional data w.r.t. the cost in the `catchException` and `asSpreader` he observes. A few observations from the performance analysis are the following: 1. The `catchException` combination is costly for setup. 2. Funneling the target method handle through `MethodHandles::asSpreader` costs another overhead. Dropping the `catchException` and specializing for a few arguments improve the cold startup for ~3x when measuring with `ReflectionColdstartBenchmark` you had [1]. Specializing to remove the asSpreader combinator also improves slightly on the throughtput. Claes can say more about the performance improvement without the spreader. 3. Splitting the virtual and static methods also improve on the throughtput. `isIllegalArgument` is not ideal but I found it acceptable as the solution for this work because it's on the exception case and depends on the implementation of core reflection and method handle exception when types mismatch. We also have tests to validate these cases. I think we should look into longer-term solution in reducing the overhead of the combinators that will benefit not only the core reflection code but also the clients of method handles. [1] http://cr.openjdk.java.net/~plevart/jdk-dev/6824466_MHReflectionAccessors/ReflectionColdstartBenchmark.java - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v2]
On Sat, 7 Aug 2021 02:07:24 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor test cleanup Great to see this come along! It's been a while since I worked on this (I've been on parental leave for a couple of months), but I plan to do a full review, re-run all benchmarks and verify that performance is where we want it to be. So I don't have much details to add to this as of right now. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v2]
On Sat, 7 Aug 2021 02:07:24 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor test cleanup Are there any user-visible aspects of this re-implementation that would merit a CSR for behavioral changes? - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v2]
On Sat, 7 Aug 2021 02:07:24 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor test cleanup There is no user-visible aspect. I plan to file a CSR to document the workaround to enable the old implementation when running into the issues described in the risks and assumption section of JEP 416. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: Remove separate accessor for static vs instance method There is no effective difference when using MethodHandle::dropArgument for static method. Removing Static*Accessor and Instance*Accessor simplifies the implementation. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/b73a6faf..cff856f9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=01-02 Stats: 809 lines in 15 files changed: 235 ins; 422 del; 152 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Sat, 21 Aug 2021 22:37:05 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Remove separate accessor for static vs instance method > > There is no effective difference when using MethodHandle::dropArgument for > static method. Removing Static*Accessor and Instance*Accessor simplifies > the implementation. [cff856f](https://github.com/openjdk/jdk/pull/5027/commits/cff856f9df89293cbc8f2f1e977148cd6ece4f85) removed the distinct classes to specialize for static vs instance methods simplify the `DirectMethodAccessorImpl` implementation and its subclasses. There is no effective difference for static method invocation to drop the first argument via MethodHandle::dropArgument or special case it in `StaticMethodAccessor::invoke`. The cold startup time increases 4ms from 92.73ms to 96.80ms when MethodHandle::dropArgument is used. Current implementation Benchmark Mode Cnt Score Error Units ReflectionMethods.instance_method avgt 10 14.902 ? 0.031 ns/op ReflectionMethods.instance_method_3arg avgt 10 18.099 ? 0.632 ns/op ReflectionMethods.instance_method_var avgt 10 14.670 ? 0.044 ns/op ReflectionMethods.instance_method_var_3arg avgt 10 18.038 ? 0.109 ns/op ReflectionMethods.static_method avgt 10 20.140 ? 0.032 ns/op ReflectionMethods.static_method_3argavgt 10 26.999 ? 0.046 ns/op ReflectionMethods.static_method_var avgt 10 22.272 ? 0.153 ns/op ReflectionMethods.static_method_var_3argavgt 10 26.567 ? 0.088 ns/op No special casing for static vs instance methods Benchmark Mode Cnt Score Error Units ReflectionMethods.instance_method avgt 10 15.177 ? 0.032 ns/op ReflectionMethods.instance_method_3arg avgt 10 17.906 ? 0.050 ns/op ReflectionMethods.instance_method_var avgt 10 14.759 ? 0.083 ns/op ReflectionMethods.instance_method_var_3arg avgt 10 18.054 ? 0.197 ns/op ReflectionMethods.static_method avgt 10 13.599 ? 0.029 ns/op ReflectionMethods.static_method_3argavgt 10 16.910 ? 0.099 ns/op ReflectionMethods.static_method_var avgt 10 13.993 ? 0.189 ns/op ReflectionMethods.static_method_var_3argavgt 10 17.555 ? 0.068 ns/op $ perf stat -r 50 jep-build/linux-x86_64-server-release/images/jdk/bin/java -cp $MICRO_JAR org.openjdk.bench.java.lang.reflect.ReflectionColdstartBenchmark Performance counter stats for 'build/linux-x86_64-server-release/images/jdk/bin/java -cp build/linux-x86_64-server-release/images/test/micro/benchmarks.jar org.openjdk.bench.java.lang.reflect.ReflectionColdstartBenchmark' (50 runs): 138.361238 task-clock:u (msec) #1.492 CPUs utilized ( +- 0.79% ) 0 context-switches:u#0.000 K/sec 0 cpu-migrations:u #0.000 K/sec 4029 page-faults:u #0.029 M/sec ( +- 0.12% ) 165249965 cycles:u #1.194 GHz ( +- 0.85% ) 187374236 instructions:u#1.13 insn per cycle
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Sat, 21 Aug 2021 22:37:05 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Remove separate accessor for static vs instance method > > There is no effective difference when using MethodHandle::dropArgument for > static method. Removing Static*Accessor and Instance*Accessor simplifies > the implementation. Peter, I have a patch [1] that adds `-Djdk.reflect.useSpreader` and `-Djdk.reflect.useCatchExceptionCombinator` flags for doing performance comparison using `asSpreader` without specializing methods with <= 3 argument and using the `catchException` combinator.Note that [1] does not change the exception handling code as I keep it simple just for performance discussion. The main drive of the specialization and not using `catchException` is the cold startup regression. Note that we run `ReflectionColdstartBenchmark` without JMH. Running it with JMH may overly skew results. I increase the number running `ReflectionColdstartBenchmark` with JMH and without JMH below. The use of `asSpreader` increases the cold startup time by ~5ms, from 98ms to 103ms whereas the use of `catchException` increases the cold startup time by ~17ms, from 98ms to ~115ms. For the instance_method* and static_method* benchmarks, we need to look at the disassembly and understand where the performance differences come from. One idea we'd like to explore is to have a special decoding logic in the spreader that can wrap NPE and CCE with IAE for core reflection use. This will remove the need of decoding the exception's stacktrace and using the `catchException` combinator. Core reflection can use the spreader with a special decoding logic. I'd propose the current implementation as the initial integration for this JEP and explore the spreader special logic and other incremental performance improvement in the core method handle implementation like in spreader or `catchException` combinator after integration. What do you think? [1] http://cr.openjdk.java.net/~mchung/jep416/useSpreader-catchException-combinator.patch -Djdk.reflect.useSpreader=false -Djdk.reflect.useCatchExceptionCombinator=false ReflectionMethods.instance_method avgt 1015.419 ? 0.026 ns/op ReflectionMethods.instance_method_3arg avgt 1019.991 ? 0.087 ns/op ReflectionMethods.instance_method_var avgt 1015.325 ? 0.048 ns/op ReflectionMethods.instance_method_var_3arg avgt 1022.998 ? 0.093 ns/op ReflectionMethods.static_method avgt 1013.927 ? 0.038 ns/op ReflectionMethods.static_method_3argavgt 1018.040 ? 0.081 ns/op ReflectionMethods.static_method_var avgt 1014.165 ? 0.048 ns/op ReflectionMethods.static_method_var_3argavgt 1017.573 ? 0.160 ns/op ReflectionColdstartBenchmark.invokeMethodsss 10 2096.975 ? 90.331 us/op -Djdk.reflect.useSpreader=true -Djdk.reflect.useCatchExceptionCombinator=false ReflectionMethods.instance_method avgt 1023.217 ? 0.058 ns/op ReflectionMethods.instance_method_3arg avgt 1027.676 ? 0.223 ns/op ReflectionMethods.instance_method_var avgt 1025.428 ? 0.038 ns/op ReflectionMethods.instance_method_var_3arg avgt 103
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Sat, 21 Aug 2021 22:37:05 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Remove separate accessor for static vs instance method > > There is no effective difference when using MethodHandle::dropArgument for > static method. Removing Static*Accessor and Instance*Accessor simplifies > the implementation. Right, early on splitting instance and static accessor implementations looked like a win for at least the instance accessor on micros, but it seems much of this win gets eaten up in more complex benchmarks since the hot call-site in `Method.invoke` sees more implementations and will be penalized by becoming bi- or megamorphic. Merging the split implementations back and accepting the need for a `dropArguments` combinator in the static case appears to be the right choice for maintainability and peak performance. A bit more overhead setting up - especially when reflecting on static methods (which is all the cold start benchmark does) - but I think that's the right trade-off in this case. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Sat, 21 Aug 2021 22:37:05 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Remove separate accessor for static vs instance method > > There is no effective difference when using MethodHandle::dropArgument for > static method. Removing Static*Accessor and Instance*Accessor simplifies > the implementation. src/java.base/share/classes/jdk/internal/reflect/DirectConstructorAccessorImpl.java line 46: > 44: // fast invoker > 45: var mhInvoker = newMethodHandleInvoker(ctor, target); > 46: return new DirectConstructorAccessorImpl(ctor, target); The created fast invoker should be passed to the ctor than discarded. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Sun, 22 Aug 2021 01:53:40 GMT, liach wrote: >> Mandy Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove separate accessor for static vs instance method >> >> There is no effective difference when using MethodHandle::dropArgument for >> static method. Removing Static*Accessor and Instance*Accessor simplifies >> the implementation. > > src/java.base/share/classes/jdk/internal/reflect/DirectConstructorAccessorImpl.java > line 46: > >> 44: // fast invoker >> 45: var mhInvoker = newMethodHandleInvoker(ctor, target); >> 46: return new DirectConstructorAccessorImpl(ctor, target); > > The created fast invoker should be passed to the ctor than discarded. Good catch. Will fix. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Sat, 21 Aug 2021 22:37:05 GMT, Mandy Chung wrote:
>> This reimplements core reflection with method handles.
>>
>> For `Constructor::newInstance` and `Method::invoke`, the new implementation
>> uses `MethodHandle`. For `Field` accessor, the new implementation uses
>> `VarHandle`.For the first few invocations of one of these reflective
>> methods on a specific reflective object we invoke the corresponding method
>> handle directly. After that we spin a dynamic bytecode stub defined in a
>> hidden class which loads the target `MethodHandle` or `VarHandle` from its
>> class data as a dynamically computed constant. Loading the method handle
>> from a constant allows JIT to inline the method-handle invocation in order
>> to achieve good performance.
>>
>> The VM's native reflection methods are needed during early startup, before
>> the method-handle mechanism is initialized. That happens soon after
>> System::initPhase1 and before System::initPhase2, after which we switch to
>> using method handles exclusively.
>>
>> The core reflection and method handle implementation are updated to handle
>> chained caller-sensitive method calls [1] properly. A caller-sensitive
>> method can define with a caller-sensitive adapter method that will take an
>> additional caller class parameter and the adapter method will be annotated
>> with `@CallerSensitiveAdapter` for better auditing. See the detailed
>> description from [2].
>>
>> Ran tier1-tier8 tests.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8013527
>> [2]
>> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove separate accessor for static vs instance method
>
> There is no effective difference when using MethodHandle::dropArgument for
> static method. Removing Static*Accessor and Instance*Accessor simplifies
> the implementation.
Overall, seems like a solid piece of work. I did not review in full the
intricacies with caller sensitive (as I don't know that area too much), but the
general rewrite seems solid.
One thing I had trouble with was the naming of the various method accessors -
for instance, DirectMethodAccessorImpl vs. NativeMethodAccessorImpl vs.
DelegatingMethodAccessorImpl vs. AdaptiveMethodAccessor (and there's more).
It's not always easy to grasp from the method name which one is new and which
one is old. Maybe sprinkling the `Handle` word on any accessor impl would help
a bit with that.
Similarly, I found the `ClassByteBuilder` name a bit weak, since that is meant
to only create instance of custom MHInvokers. A more apt name will help there
too.
I also had some issues parsing the flow in
`ReflectionFactory::newMethodAccessor` (and constructor, has similar issue):
if (useNativeAccessor(method)) {
return DirectMethodAccessorImpl.nativeAccessor(method,
callerSensitive);
}
return MethodHandleAccessorFactory.newMethodAccessor(method,
callerSensitive);
```
Why can't logic like this be encapsulated in a single factory call? E.g. it
seems like the code is would like to abstract the differences between the
various accessor implementations, but it doesn't seem to get all the way there
(it's also possible I'm missing some constraint here).
src/java.base/share/classes/jdk/internal/reflect/DirectConstructorAccessorImpl.java
line 106:
> 104: Object invokeImpl(Object[] args) throws Throwable {
> 105: var mhInvoker = mhInvoker();
> 106: return switch (paramCount) {
As @plevart observed, I'm also a bit surprised to see this, but I note your
comments regarding performance - especially in cold case. Every adaptation
counts, I guess, if you're not in the hot path. But let's make sure that the
pay off to add the extra hand-specialized cases is worth it - I'd be surprised
if spreading arguments is that expensive.
src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
200:
> 198: return MethodHandleAccessorFactory.newMethodAccessor(method,
> callerSensitive);
> 199: } else {
> 200: if (!useDirectMethodHandle && noInflation
seems to me that the `!useDirectMethodHandle` is useless here (always false) ?
src/java.base/share/classes/jdk/internal/reflect/VarHandleBooleanFieldAccessorImpl.java
line 32:
> 30: import java.lang.reflect.Modifier;
> 31:
> 32: abstract class VarHandleBooleanFieldAccessorImpl extends
> VarHandleFieldAccessorImpl {
I wondered if we could avoid these hand specialized versions of VH accessors
(one for each carrier type) by instead getting the getter/setter method handle
out of the var handle, and adapt that so that its type matches Object... but
then I realized that the `Field` API has sharp types in the gett
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Fri, 27 Aug 2021 13:28:08 GMT, Maurizio Cimadamore
wrote:
> Overall, seems like a solid piece of work. I did not review in full the
> intricacies with caller sensitive (as I don't know that area too much), but
> the general rewrite seems solid.
Thanks for the review.
> One thing I had trouble with was the naming of the various method accessors -
> for instance, DirectMethodAccessorImpl vs. NativeMethodAccessorImpl vs.
> DelegatingMethodAccessorImpl vs. AdaptiveMethodAccessor (and there's more).
> It's not always easy to grasp from the method name which one is new and which
> one is old. Maybe sprinkling the `Handle` word on any accessor impl would
> help a bit with that.
I can see how this could be confusing. Sprinkling the `Handle` word may help.
I will try that.
> Similarly, I found the `ClassByteBuilder` name a bit weak, since that is
> meant to only create instance of custom MHInvokers. A more apt name will help
> there too.
What about `InvokerBuilder` (it creates either MHInvoker or VHInvoker)?
> I also had some issues parsing the flow in
> `ReflectionFactory::newMethodAccessor` (and constructor, has similar issue):
>
> ```
>if (useNativeAccessor(method)) {
> return DirectMethodAccessorImpl.nativeAccessor(method,
> callerSensitive);
> }
> return MethodHandleAccessorFactory.newMethodAccessor(method,
> callerSensitive);
> ```
>
> Why can't logic like this be encapsulated in a single factory call? E.g. it
> seems like the code is would like to abstract the differences between the
> various accessor implementations, but it doesn't seem to get all the way
> there (it's also possible I'm missing some constraint here).
We have to avoid to initialize the `MethodHandleAccessorFactory` class until
`java.lang.invoke` is initialized. because
`MethodHandleAccessorFactory::` creates lookup object and method
handles. I agree it's cleaner to encapsulate these in one single factory
method.I think moving the the static fields and the initialization to a
holder class may be a way to do it. I will give it a try.
> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
> 200:
>
>> 198: return
>> MethodHandleAccessorFactory.newMethodAccessor(method, callerSensitive);
>> 199: } else {
>> 200: if (!useDirectMethodHandle && noInflation
>
> seems to me that the `!useDirectMethodHandle` is useless here (always false) ?
Good catch! Will fix.
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v4]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with four additional commits since the last revision: - Rename the accessor classes to make it explicit for method handles - Add a new test for testing methods whose parameter types and return type not visible to the caller - Move the handling of native accessor to the factory method for method/constructor accessor - DirectConstructorAccessorImpl should take the MHInvoker parameter - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/cff856f9..ba099fb4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=02-03 Stats: 658 lines in 12 files changed: 401 ins; 202 del; 55 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v5]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: Shorten the class name - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/ba099fb4..475a1be6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=03-04 Stats: 23 lines in 4 files changed: 0 ins; 0 del; 23 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v6]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: Fix NativeAccessor.c build issue for the renamed classes - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/475a1be6..df6fb0a2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=04-05 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Mon, 30 Aug 2021 16:56:20 GMT, Mandy Chung wrote: > What about `InvokerBuilder` (it creates either MHInvoker or VHInvoker)? Please throw a "Reflection" somewhere :-) - e.g. "ReflectionInvokerBuilder" - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v7]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: Rename InvokerBuilder to ReflectionInvokerBuilder - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/df6fb0a2..68bb9efe Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=05-06 Stats: 12 lines in 3 files changed: 1 ins; 2 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Tue, 31 Aug 2021 16:01:39 GMT, Maurizio Cimadamore wrote: > Please throw a "Reflection" somewhere :-) - e.g. "ReflectionInvokerBuilder" Done. I'm fine with that. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v7]
On Tue, 31 Aug 2021 17:14:05 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Rename InvokerBuilder to ReflectionInvokerBuilder Thanks for the changes - they look good and they make the code initialization logic easier to follow. Looks good to me. - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Fri, 27 Aug 2021 13:12:33 GMT, Maurizio Cimadamore
wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove separate accessor for static vs instance method
>>
>> There is no effective difference when using MethodHandle::dropArgument for
>> static method. Removing Static*Accessor and Instance*Accessor simplifies
>> the implementation.
>
> src/java.base/share/classes/jdk/internal/reflect/DirectConstructorAccessorImpl.java
> line 106:
>
>> 104: Object invokeImpl(Object[] args) throws Throwable {
>> 105: var mhInvoker = mhInvoker();
>> 106: return switch (paramCount) {
>
> As @plevart observed, I'm also a bit surprised to see this, but I note your
> comments regarding performance - especially in cold case. Every adaptation
> counts, I guess, if you're not in the hot path. But let's make sure that the
> pay off to add the extra hand-specialized cases is worth it - I'd be
> surprised if spreading arguments is that expensive.
While I recall it was motivated primarily for startup (note startup numbers for
`-Djdk.reflect.useSpreader=true` in @mlchung reply earlier in this PR), the
specialization to avoid the spreader for low-arity arguments also improve
performance on throughput microbenchmarks. Surprisingly also reduce the per
invocation allocation rate.
Allocation profiling suggests that with the specialization a varargs array is
entirely eliminated. This is of course somewhat fragile and dependent on a
number of things - and might ultimately be an artifact of a rather synthetic
microbenchmark that will have little benefit in practice, but it's a rather
consistent gain for various number of arguments - even when actually going into
a spreader (I have some hypotheses as to why this might happen, but most likely
we help the JIT to narrow things down for each kind of invocation scheme with
the selector):
18 baseline
Benchmark Mode
CntScore Error Units
ReflectionMethods.static_methodavgt
10 57.329 ± 4.217 ns/op
ReflectionMethods.static_method:·gc.alloc.rate.normavgt
10 72.006 ± 0.001B/op
ReflectionMethods.static_method_3arg avgt
10 70.940 ± 7.457 ns/op
ReflectionMethods.static_method_3arg:·gc.alloc.rate.norm avgt
10 96.008 ± 0.002B/op
ReflectionMethods.static_method_4arg avgt
10 90.453 ± 4.252 ns/op
ReflectionMethods.static_method_4arg:·gc.alloc.rate.norm avgt
10 112.010 ± 0.001B/op
pull/5027
Benchmark Mode
CntScore Error Units
ReflectionMethods.static_methodavgt
10 41.518 ± 2.444 ns/op
ReflectionMethods.static_method:·gc.alloc.rate.normavgt
10 48.004 ± 0.001B/op
ReflectionMethods.static_method_3arg avgt
10 57.603 ± 3.299 ns/op
ReflectionMethods.static_method_3arg:·gc.alloc.rate.norm avgt
10 64.006 ± 0.001B/op
ReflectionMethods.static_method_4arg avgt
10 56.772 ± 3.971 ns/op
ReflectionMethods.static_method_4arg:·gc.alloc.rate.norm avgt
10 80.007 ± 0.001B/op
On a patch to remove the specialization on top of pull/5027, performance
reverts back to numbers very similar to the baseline:
remove-spreader-patch:
Benchmark Mode
CntScore Error Units
ReflectionMethods.static_methodavgt
10 56.644 ± 4.322 ns/op
ReflectionMethods.static_method:·gc.alloc.rate.normavgt
10 72.006 ± 0.001B/op
ReflectionMethods.static_method_3arg avgt
10 72.353 ± 6.815 ns/op
ReflectionMethods.static_method_3arg:·gc.alloc.rate.norm avgt
10 96.008 ± 0.001B/op
ReflectionMethods.static_method_4arg avgt
10 82.820 ± 8.303 ns/op
ReflectionMethods.static_method_4arg:·gc.alloc.rate.norm avgt
10 112.009 ± 0.002B/op
I think the cold start reduction alone is worth the extra 100 lines or so of
code, but if the gain on these microbenchmarks translates to real throughput
gains then I think the complexity is more than paid for. Simplifying it while
retaining similar characteristics would be great of course, but I think such an
exploration should be done as a follow-up.
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: minor cleanup and more test case. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/68bb9efe..32e7f340 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=06-07 Stats: 56 lines in 8 files changed: 33 ins; 0 del; 23 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. This is great work! There is more specialization than I would have expected but it looks maintainable so I think should be okay to integrate with that and investigate a special spreader later. I think it would be good to get this into JDK 18 early so that there is lots of time to test and identify corner cases with performance. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. Hi Mandy, I'm still looking at this great work and have a few questions that I want to ask upfront. I understand the need for hard-coded specialization as opposed to inserting more MH transformations. You want to squeeze the startup costs as much as possible. But what I would like to understand is the need for MHInvoker and generated implementations of that interface. I can see that by making the target MH a constant in such generated MHInvoker, JVM is able to optimize the MH invocation chain better when JIT kicks-in. So instead of holding the reference to a target MethodHandle in say DirectMethodHandleAccessor, you hold a reference to MHInvoker. You trade constant MH optimization for indirection to a non-constant MHInvoker instance (I see @Stable annotation there, but it would only be effective when the holder MethodAccessor instance was also constant which unfortunately isn't as it is held by a volatile field in Method so even if Method was constant, its MethodAccessor would not be). So my question is whether this trade pays off. I wonder what the performance would be if: - MHInvoker was eliminated - the DirectMethodHandleAccessor just used the target MH directly (via @Stable field) but still using hard-coded specializations - AdaptiveMethodHandleAccessor would not be needed then - the Method had @Stable reference to MethodAccessor instead of volatile (data races are used everywhere so why not for this field too?) I guess you already tried this approach and later added MHInvoker "middleman" to optimize the warmed-up performance. If not, I can try that variant and come up with benchmark results to compare... - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. Hi Peter, I haven't tried the approach of making the Method @stable reference to MethodAccessor instead of volatile. This reminds me of your experiment [1] which I lost track of. It'd be good if you can try that approach and compare the benchmark results. [1] https://bugs.openjdk.java.net/browse/JDK-6824466?focusedCommentId=14212580&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14212580 - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. Hi Mandy, So, here's the mentioned approach: https://github.com/plevart/jdk/commit/1a31508b7d73bd9fa40aae845c06b24c3cbcfd16 This is a commit above your changes that I have picked from your branch and squashed as a single commit above current master: https://github.com/plevart/jdk/commit/a2e4a1f9dad9e6951949ce82af54639ed2da9ddf I think you could chery-pick the former commit into your branch if you wanted to. What I have done is mainly removing all code around MHInvoker and VHInvoker and calling the target MethodHandle/VarHandle directly in the XxxAccessors. I also made sure that the path of references going from Method/Constructor/Field via MethodAccessor(s)/ConstructorAccessor(s)/FieldAccessor(s) and to MethodHandle/VarHandle is marked with @Stable. So if the Method/Constructor/Field is found to be constant by JIT, so is its associated MethodHandle/VarHandle. I also "fixed" existing MethodAccessor(s)/ConstructorAccessor(s)/FieldAccessor(s) so that they are safe to be "published" via data-race as the fields holding them in Method/Constructor are no longer volatile (in Field they never have been volatile). In Field I had to make two distinct call-sites for fieldAccessor vs. overrideFieldAccessor in order for JIT to propagate @Stable-ness down the chain. I then ran the following JMH benchmarks (not included in my variant of patch yet) on jdk16, mandy's variant and peter's variant: https://gist.github.com/plevart/b9c62c8cac9784e2c6d5f51a6386fa84 The results can be visualized via the following links: jdk16 vs. mandy: https://jmh.morethan.io/?gists=9219d79a09a805eb39607830270d1513,80203847700b9ab8baeb346a02efc804 jdk16 vs. peter: https://jmh.morethan.io/?gists=9219d79a09a805eb39607830270d1513,b11842fbd48ebbd9234251fded50d52e mandy vs. peter: https://jmh.morethan.io/?gists=80203847700b9ab8baeb346a02efc804,b11842fbd48ebbd9234251fded50d52e Comparing mandy vs. peter, there are pros and cons. If Method/Field instance is found to be constant by JIT, peter's variant is a little better (16...36%). It Method/Field is not constant as found by JIT, but still a single instance of Method/Field is invoked in a particular call-site, peter's variant is worse (22...48%). I added another variant called "Poly" (for static methods/fields only, but I think results would be similar for instance too). In this variant a single call-site operates with distinct instances of Method/Field and peter's variant is a little better there (3...11%). The cold-start is a little better for peter's variant too (5%). I think the "Poly" variants are an important representative of real-world usage. Every usage happens in the real world (Const, Var, Poly), but most applications/frameworks that use reflection and where performance matters (such as JPA, various serialization frameworks, etc.) are generic algorithm(s) that abstract over various runtime types, accessing methods/fields/constructors via call-sites that deal with multiple instances of Mehod/Field/Constructor each. Top speed is possible with peter's variant and even better than with mandys's variant when each call-
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. Just another question: Is migrating from Unsafe based FieldAccessor(s) to VarHandle based an important step to final goal of Unsafe elimination because I don't think it is a necessary step for Loom. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. Ah, I found some late copy-paste mistakes. Here's the commit (on top of peter's variant commit mentioned above): https://github.com/plevart/jdk/commit/791a9a69967674836fcfc41746dc731396f3bb3e - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Tue, 14 Sep 2021 19:33:38 GMT, Peter Levart wrote: > Just another question: Is migrating from Unsafe based FieldAccessor(s) to > VarHandle based an important step to final goal of Unsafe elimination because > I don't think it is a necessary step for Loom. No. We could continue to use Unsafe but moving to VarHandle reduces the development cost when extending core reflection for a new feature for example for Project Valhalla, which is one of the goals. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. For the performance comparison, the current baseline for this PR is jdk-18+9 that will better reflect the performance gain/regression. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. > Ah, I found some late copy-paste mistakes. Here's the commit (on top of > peter's variant commit mentioned above): > [plevart@791a9a6](https://github.com/plevart/jdk/commit/791a9a69967674836fcfc41746dc731396f3bb3e) Thanks for catching it. Will fix. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. Ok, here are comparisons with jdk18+9: jdk18+9 vs. mandy: https://jmh.morethan.io/?gists=7b2399cea71d12dfd3434701ddeed026,80203847700b9ab8baeb346a02efc804 jdk18+9 vs. peter: https://jmh.morethan.io/?gists=7b2399cea71d12dfd3434701ddeed026,b11842fbd48ebbd9234251fded50d52e mandy vs. peter: https://jmh.morethan.io/?gists=80203847700b9ab8baeb346a02efc804,b11842fbd48ebbd9234251fded50d52e jdk16 vs. jdk18+9: https://jmh.morethan.io/?gists=9219d79a09a805eb39607830270d1513,7b2399cea71d12dfd3434701ddeed026 As can be seen from the jdk16 vs. jdk18+9, jdk18+9 has improved mainly on `staticMethodConst`, but peter's variant still beats it by large margin. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. Hi Peter, thanks for the patch and performance measurement. Yesterday I did a quick hack and observed similar result. I'm excited to see the performance improvement in the `Const` cases without the need of spinning the hidden class and it's a simplification. On the other hand, I'm concerned of the regression of the `Var` case for field access (80%) whereas the regression of the `Var` case for method may be tolerable (9-10%) [1]. We don't have a representative benchmark for core reflection that represents the performance charactistics of real-world applications/frameworks. I can't make a conclusion whether regression of `Var` cases matter in real-world applications/frameworks until customers run their benchmarks. We have to take the conservative side to ensure no significant regression based on micro-benchmarks result. I'm afraid it's not a good state to accept. Claes is on vacation and we should get his opinion. I tried one suggestion from Vladimir Ivanov to ensure `MethodHandle::customize` on top of your patch. But it does not seem to help. In any case, the patch to make `methodAccessor` and `fieldAccessor` not volatile is a good improvement by itself (not to be a volatile field). I would suggest to fix that separately from JEP 416. For JEP 416, two possible options: 1. Keep it as is and no significant performance regression. Continue to work on the performance improvement to remove the need of spinning class. 2. Disable the spinning hidden class and have a flag to enable it so that customers can workaround if having performance issue. However, performance regression on field access is not desirable. So I would prefer option 1 and continue work on the performance improvement as follow-up work. What do you think? [1] https://jmh.morethan.io/?gists=7b2399cea71d12dfd3434701ddeed026,b11842fbd48ebbd9234251fded50d52e - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. Hi Peter, `VarHandles` are not backed by `LambdaForm`. I did an experiment to change the `FieldAccessor` to use `MethodHandle` instead of `VarHandle` [1] applying on top of your patch. MH customization does help improving the performance. The regression is reduced from 82-85% to 43-52%. Perhaps this is more acceptable and see what @cl4es thinks. baseline Benchmark Mode Cnt Score Error Units ReflectionSpeedBenchmark.instanceFieldConst avgt 10 80.557 ? 0.022 ns/op ReflectionSpeedBenchmark.instanceFieldVar avgt 10 73.698 ? 0.458 ns/op ReflectionSpeedBenchmark.staticFieldConst avgt 10 59.393 ? 0.028 ns/op ReflectionSpeedBenchmark.staticFieldPoly avgt 10 102.994 ? 0.205 ns/op ReflectionSpeedBenchmark.staticFieldVar avgt 10 67.911 ? 0.061 ns/op plevart's patch: Benchmark Mode Cnt Score Error Units ReflectionSpeedBenchmark.instanceFieldConst avgt 1042.429 ? 0.016 ns/op ReflectionSpeedBenchmark.instanceFieldVar avgt 10 133.405 ? 0.087 ns/op ReflectionSpeedBenchmark.staticFieldConst avgt 1042.468 ? 0.326 ns/op ReflectionSpeedBenchmark.staticFieldPoly avgt 10 188.546 ? 0.073 ns/op ReflectionSpeedBenchmark.staticFieldVar avgt 10 124.686 ? 0.078 ns/op plevart's patch + vh-mh.patch [1] Benchmark Mode Cnt Score Error Units ReflectionSpeedBenchmark.instanceFieldConst avgt 1042.402 ? 0.022 ns/op ReflectionSpeedBenchmark.instanceFieldVar avgt 10 105.648 ? 0.047 ns/op ReflectionSpeedBenchmark.staticFieldConst avgt 1042.437 ? 0.033 ns/op ReflectionSpeedBenchmark.staticFieldPoly avgt 10 178.820 ? 0.292 ns/op ReflectionSpeedBenchmark.staticFieldVar avgt 10 102.676 ? 0.081 ns/op [1] http://cr.openjdk.java.net/~mchung/jep416/vh-mh.patch - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. Hi Mandy, Yes, you had exactly the same thoughts as me! So this improves the "Var" case for fields. I also ran tests on my PC with your vh-mh patch on top and here are visualized results (combined with previous results): jdk18+9 vs. mandy: https://jmh.morethan.io/?gists=7b2399cea71d12dfd3434701ddeed026,80203847700b9ab8baeb346a02efc804 jdk18+9 vs. peter: https://jmh.morethan.io/?gists=7b2399cea71d12dfd3434701ddeed026,b11842fbd48ebbd9234251fded50d52e jdk18+9 vs. peter+vh2mh: https://jmh.morethan.io/?gists=7b2399cea71d12dfd3434701ddeed026,2a61a99909415f9e0ab53b125628b11d mandy vs. peter: https://jmh.morethan.io/?gists=80203847700b9ab8baeb346a02efc804,b11842fbd48ebbd9234251fded50d52e mandy vs. peter+vh2mh: https://jmh.morethan.io/?gists=80203847700b9ab8baeb346a02efc804,2a61a99909415f9e0ab53b125628b11d peter vs. peter+vh2mh: https://jmh.morethan.io/?gists=b11842fbd48ebbd9234251fded50d52e,2a61a99909415f9e0ab53b125628b11d - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. My opinion is that "Const" and "Poly" are the use cases that matter and "Var" is really very minor case. Unfortunately the "Poly" case for fields has regression comparing to Unsafe accessors in all our patches. The least regression (35%) has peter+vh2mh which also has the best improvement in "Const" use cases for fields and methods (16...45%). I can prepare a real-world test case (JSON serialization framework Jackson for example) to compare results using such benchmark. WDYT? - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. So here's a benchmark that measures the impact on a real-world use case - the Jackson (de)serialization: https://gist.github.com/plevart/3cdc7c366d03822c915a7b3ccd579421 Visualized comparisons: jdk18+9 vs. mandy: https://jmh.morethan.io/?gists=f81f8e67ec74c3a5c8110a60ddbf38d7,e41df754bfb07df65714b113f7ed08f3 jdk18+9 vs. peter+vh2mh: https://jmh.morethan.io/?gists=f81f8e67ec74c3a5c8110a60ddbf38d7,9bf7e119ffb859e498772f47ba68c709 mandy vs. peter+vh2mh: https://jmh.morethan.io/?gists=e41df754bfb07df65714b113f7ed08f3,9bf7e119ffb859e498772f47ba68c709 Not so much of an impact on a real-world case as artificial benchmarks. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. Thanks for doing the Jackson (de)serialization benchmark. I'll follow up with Claes when he returns from vacation end of this week and see if we can agree that performance improvements can be explored as follows-up works. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. I'm not sure how to assess how minor the "Var" case really is. I wouldn't be surprised if reflection-heavy frameworks hold `Method`s etc in some collection, which means they wouldn't be rooted in a way that allows the JIT to fold through. Thus leaning only on MH customization could be adding some performance risks. Off-list Vladimir Ivanov suggested the "Var" micros have some issues with inlining that make them look worse than they should, though. On balance I think removing class-spinning might mean a better overall story w.r.t. footprint and maintainability. Had we started this review using a patch that looked more like what Peter is suggestion and someone had suggested we spin classes to get a performance edge in a subset of cases I think we'd not be looking favorably at that and instead tried reaching for narrowing those performance gaps via other less intrusive means. So I think I'm in favor of simplifying and filing a follow-up to try and win back some of the performance we might be losing on corner-case micros without the custom class spinning. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. Hi Claes, > I'm not sure how to assess how minor the "Var" case really is. I wouldn't be > surprised if reflection-heavy frameworks hold `Method`s etc in some > collection, which means they wouldn't be rooted in a way that allows the JIT > to fold through. Thus leaning only on MH customization could be adding some > performance risks. Off-list Vladimir Ivanov suggested the "Var" micros have > some issues with inlining that make them look worse than they should, though. Frameworks that keep Method(s) etc in some collection don't fall into the "Var" category of MH tests presented here, but into the "Poly" category. The test that uses Jackson serialization is one such example. The "Var" class of tests exhibit a use-case where there is a single instance of Method (or Field) object used in a particular call-site (i.e. the Method::invoke invocation in bytecode) but such Method object can't be proved by JIT to be constant (it is not read from static final or @Stable field). I doubt that such use-cases exist in practice. Mostly they would amount to cases that were meant to be "Constant" but are not because the user forgot to use "final" modifier on "static" field... If you look at "Poly" results, spinning MHInvoker/VHInvoker classes for each instance of Method/Field does not help at all. I would try to optimize the "Poly" case further if it was possible. The simplified Method/MethodHandle is practically equivalent in final "Poly" performance with the generated MethodAccessor, but the Field/VarHandle or Field/MethodHandle lags behind Unsafe-based accessor in "Poly" performance. Nothing can beat Unsafe when access to fields is concerned. It doesn't matter where the offset and base are read-from, the access is super-fast. I wish MethodHandles obtained by unreflectGetter/unreflectSetter could be less sensitive to where they are read from (constant vs. not-constant) and optimize better in non-constant case. I think we should search in this direction... to make MethodHandles obtained by unreflectGetter/unreflectSetter more optimal in non-constant case. If Unsafe can be better, why not MethodHandles? > > On balance I think removing class-spinning might mean a better overall story > w.r.t. footprint and maintainability. Had we started this review using a > patch that looked more like what Peter is suggestion and someone had > suggested we spin classes to get a performance edge in a subset of cases I > think we'd not be looking favorably at that and instead tried reaching for > narrowing those performance gaps via other less intrusive means. So I think > I'm in favor of simplifying and filing a follow-up to try and win back some > of the performance we might be losing on corner-case micros without the > custom class spinning. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. > Frameworks that keep Method(s) etc in some collection don't fall into the > "Var" category of MH tests presented here, but into the "Poly" category. The > test that uses Jackson serialization is one such example. The "Var" class of > tests exhibit a use-case where there is a single instance of Method (or > Field) object used in a particular call-site (i.e. the Method::invoke > invocation in bytecode) but such Method object can't be proved by JIT to be > constant (it is not read from static final or @stable field). I doubt that > such use-cases exist in practice. Mostly they would amount to cases that were > meant to be "Constant" but are not because the user forgot to use "final" > modifier on "static" field... > > If you look at "Poly" results, spinning MHInvoker/VHInvoker classes for each > instance of Method/Field does not help at all. The added indirection in the "Poly" test might cause the code to fall off the inlining cliff, so that any constant folding becomes unlikely to happen. I agree that this might be closer to what happens in a real app, and as a data point it does show that the MHInvoker/VHInvoker optimizations might have been misguided. > > I would try to optimize the "Poly" case further if it was possible. The > simplified Method/MethodHandle is practically equivalent in final "Poly" > performance with the generated MethodAccessor, but the Field/VarHandle or > Field/MethodHandle lags behind Unsafe-based accessor in "Poly" performance. > Nothing can beat Unsafe when access to fields is concerned. It doesn't matter > where the offset and base are read-from, the access is super-fast. I wish > MethodHandles obtained by unreflectGetter/unreflectSetter could be less > sensitive to where they are read from (constant vs. not-constant) and > optimize better in non-constant case. I think we should search in this > direction... to make MethodHandles obtained by > unreflectGetter/unreflectSetter more optimal in non-constant case. If Unsafe > can be better, why not MethodHandles? Completely agree, and it makes sense to me to try and optimize MH/VH field access to get on par with Unsafe for these use-cases rather than revert the reflective implementation back to raw Unsafe use. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Tue, 21 Sep 2021 15:06:49 GMT, Claes Redestad wrote:
> > If you look at "Poly" results, spinning MHInvoker/VHInvoker classes for
> > each instance of Method/Field does not help at all.
>
> The added indirection in the "Poly" test might cause the code to fall off the
> inlining cliff, so that any constant folding becomes unlikely to happen. I
> agree that this might be closer to what happens in a real app, and as a data
> point it does show that the MHInvoker/VHInvoker optimizations might have been
> misguided.
It is not the added indirection (I can add an indirection in the "Var" case too
and benchmark would not change), but the variation of Method/Field instances
used at the same call-site. When there is only one instance ("Var" case),
delegating invocation of a MH to a MHInvoker somehow optimizes speculatively
with a guard that just checks that the same MHInvoker class is used each time
and further code is generated with constant MH.
Such speculative optimization does not happen with non-constant MHs
unfortunately despite the fact that there is only one MH instance.
But in "Poly" case, generated MHInvoker classes don't help as there are
multiple such classes and the speculative optimization falls apart (or is never
even tried).
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. > On balance I think removing class-spinning might mean a better overall story > w.r.t. footprint and maintainability. > : > So I think I'm in favor of simplifying and filing a follow-up to try and win > back some of the performance we might be losing on corner-case micros without > the custom class spinning. Great! I'll update the PR with Peter's patch + vh2mh with removal of StaticFieldAccessor/InstanceFieldAccessor inner classes [2] (no significant performance difference between [1] and [2]). > I wish MethodHandles obtained by unreflectGetter/unreflectSetter could be > less sensitive to where they are read from (constant vs. not-constant) and > optimize better in non-constant case. I think we should search in this > direction... to make MethodHandles obtained by > unreflectGetter/unreflectSetter more optimal in non-constant case. Yup, that's what the performance opportunity we should explore to improve MH/VH that will benefit all clients of MH/VH as well as core reflection. [1] http://cr.openjdk.java.net/~mchung/jep416/vh-mh.patch [2] http://cr.openjdk.java.net/~mchung/jep416/vh-mh-2.patch - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v9]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 27 commits: - Make MethodAccessor non-volatile. Remove MHInvoker/VHInvoker. - Merge - minor cleanup and more test case. - Rename InvokerBuilder to ReflectionInvokerBuilder - Fix NativeAccessor.c build issue for the renamed classes - Shorten the class name - Rename the accessor classes to make it explicit for method handles - Add a new test for testing methods whose parameter types and return type not visible to the caller - Move the handling of native accessor to the factory method for method/constructor accessor - DirectConstructorAccessorImpl should take the MHInvoker parameter - ... and 17 more: https://git.openjdk.java.net/jdk/compare/161fdb4a...1e68d004 - Changes: https://git.openjdk.java.net/jdk/pull/5027/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=08 Stats: 6529 lines in 75 files changed: 5990 ins; 327 del; 212 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v10]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 30 commits: - Add java.lang.reflect and jdk.internal.reflect to trusted non-static final fields packages - Merge - ensure class initialized before field acces - Make MethodAccessor non-volatile. Remove MHInvoker/VHInvoker. - Merge - minor cleanup and more test case. - Rename InvokerBuilder to ReflectionInvokerBuilder - Fix NativeAccessor.c build issue for the renamed classes - Shorten the class name - Rename the accessor classes to make it explicit for method handles - ... and 20 more: https://git.openjdk.java.net/jdk/compare/83b22192...9c28df4c - Changes: https://git.openjdk.java.net/jdk/pull/5027/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=09 Stats: 6411 lines in 78 files changed: 5896 ins; 317 del; 198 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v11]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with two additional commits since the last revision: - Remove duplicated microbenchmarks - Avoid pitfall with unwanted inlining in some cases. Also remove boxing/unboxing to focus on the invocation cost - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/9c28df4c..49029aa9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=09-10 Stats: 407 lines in 3 files changed: 71 ins; 309 del; 27 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: Fix left-over assignment - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/49029aa9..86d34f48 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=10-11 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]
On Fri, 8 Oct 2021 23:31:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Fix left-over assignment [8cb8071](https://github.com/openjdk/jdk/pull/5027/commits/8cb8071d9a085349139215c8472730193650b247) adds the setup code to pollute the profile to avoid unwanted inlining in some cases. The benchmark numbers are now sensible where the `Var` cases should not perform better than `Const` cases. I observed that if `polluteProfile` is false, some `Var` cases perform better than `Const` cases. Updated performance result: Baseline (jdk-18+17) Benchmark Mode CntScoreError Units ReflectionSpeedBenchmark.constructorConst avgt 10 68.049 ± 0.872 ns/op ReflectionSpeedBenchmark.constructorPoly avgt 10 94.132 ± 1.805 ns/op ReflectionSpeedBenchmark.constructorVar avgt 10 64.543 ± 0.799 ns/op ReflectionSpeedBenchmark.instanceFieldConst avgt 10 35.361 ± 0.492 ns/op ReflectionSpeedBenchmark.instanceFieldPolyavgt 10 67.089 ± 3.288 ns/op ReflectionSpeedBenchmark.instanceFieldVar avgt 10 35.745 ± 0.554 ns/op ReflectionSpeedBenchmark.instanceMethodConst avgt 10 77.925 ± 2.026 ns/op ReflectionSpeedBenchmark.instanceMethodPoly avgt 10 96.094 ± 2.269 ns/op ReflectionSpeedBenchmark.instanceMethodVaravgt 10 80.002 ± 4.267 ns/op ReflectionSpeedBenchmark.staticFieldConst avgt 10 33.442 ± 2.659 ns/op ReflectionSpeedBenchmark.staticFieldPoly avgt 10 51.918 ± 1.522 ns/op ReflectionSpeedBenchmark.staticFieldVar avgt 10 33.967 ± 0.451 ns/op ReflectionSpeedBenchmark.staticMethodConstavgt 10 75.380 ± 1.660 ns/op ReflectionSpeedBenchmark.staticMethodPoly avgt 10 93.553 ± 1.037 ns/op ReflectionSpeedBenchmark.staticMethodVar avgt 10 76.728 ± 1.614 ns/op JEP 417 Benchmark Mode Cnt Score Error Units ReflectionSpeedBenchmark.constructorConst avgt 10 32.392 ± 0.473 ns/op ReflectionSpeedBenchmark.constructorPoly avgt 10 113.947 ± 1.205 ns/op ReflectionSpeedBenchmark.constructorVar avgt 10 76.885 ± 1.128 ns/op ReflectionSpeedBenchmark.instanceFieldConst avgt 10 18.569 ± 0.161 ns/op ReflectionSpeedBenchmark.instanceFieldPolyavgt 10 98.671 ± 2.015 ns/op ReflectionSpeedBenchmark.instanceFieldVar avgt 10 54.193 ± 3.510 ns/op ReflectionSpeedBenchmark.instanceMethodConst avgt 10 33.421 ± 0.406 ns/op ReflectionSpeedBenchmark.instanceMethodPoly avgt 10 109.129 ± 1.959 ns/op ReflectionSpeedBenchmark.instanceMethodVaravgt 10 90.420 ± 2.187 ns/op ReflectionSpeedBenchmark.staticFieldConst avgt 10 19.080 ± 0.179 ns/op ReflectionSpeedBenchmark.staticFieldPoly avgt 10 92.130 ± 2.729 ns/op ReflectionSpeedBenchmark.staticFieldVar avgt 10 53.899 ± 1.051 ns/op ReflectionSpeedBenchmark.staticMethodConstavgt 10 35.907 ± 0.456 ns/op ReflectionSpeedBenchmark.staticMethodPoly avgt 10 102.895 ± 1.604 ns/op ReflectionSpeedBenchmark.staticMethodVar avgt 10 82.123 ± 0.629 ns/op I also ran the following benchmarks which show no performance degradation: - Peter's custom JSON serialization a
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]
On Tue, 12 Oct 2021 17:42:01 GMT, Peter Levart wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix left-over assignment
>
> src/java.base/share/classes/jdk/internal/reflect/MethodHandleCharacterFieldAccessorImpl.java
> line 137:
>
>> 135: {
>> 136: if (isReadOnly()) {
>> 137: ensureObj(obj); // throw NPE if obj is null on instance
>> field
>
> I think ensureObj(obj) must go before if statement in setChar
No, it's OK. You are relying on `setter.invokeExact(obj, c)` to throw NPE
later...
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]
On Fri, 8 Oct 2021 23:31:32 GMT, Mandy Chung wrote:
>> This reimplements core reflection with method handles.
>>
>> For `Constructor::newInstance` and `Method::invoke`, the new implementation
>> uses `MethodHandle`. For `Field` accessor, the new implementation uses
>> `VarHandle`.For the first few invocations of one of these reflective
>> methods on a specific reflective object we invoke the corresponding method
>> handle directly. After that we spin a dynamic bytecode stub defined in a
>> hidden class which loads the target `MethodHandle` or `VarHandle` from its
>> class data as a dynamically computed constant. Loading the method handle
>> from a constant allows JIT to inline the method-handle invocation in order
>> to achieve good performance.
>>
>> The VM's native reflection methods are needed during early startup, before
>> the method-handle mechanism is initialized. That happens soon after
>> System::initPhase1 and before System::initPhase2, after which we switch to
>> using method handles exclusively.
>>
>> The core reflection and method handle implementation are updated to handle
>> chained caller-sensitive method calls [1] properly. A caller-sensitive
>> method can define with a caller-sensitive adapter method that will take an
>> additional caller class parameter and the adapter method will be annotated
>> with `@CallerSensitiveAdapter` for better auditing. See the detailed
>> description from [2].
>>
>> Ran tier1-tier8 tests.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8013527
>> [2]
>> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix left-over assignment
Marked as reviewed by plevart (Reviewer).
Hi Mandy,
I think this is good as first slab. I don't have anything to add at this point.
Some optimization ideas to try as followups. I ran benchmarks myself and the
results are similar to yours. Some seem like a regression, but don't have big
impact on real-world use case such as Jackson (de)serialization. "Const" class
is pretty much the same as with recently improved variant with @Stable fields.
I say Go!
src/java.base/share/classes/jdk/internal/reflect/MethodHandleCharacterFieldAccessorImpl.java
line 137:
> 135: {
> 136: if (isReadOnly()) {
> 137: ensureObj(obj); // throw NPE if obj is null on instance
> field
I think ensureObj(obj) must go before if statement in setChar
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]
On Tue, 12 Oct 2021 17:44:08 GMT, Peter Levart wrote:
>> src/java.base/share/classes/jdk/internal/reflect/MethodHandleCharacterFieldAccessorImpl.java
>> line 137:
>>
>>> 135: {
>>> 136: if (isReadOnly()) {
>>> 137: ensureObj(obj); // throw NPE if obj is null on
>>> instance field
>>
>> I think ensureObj(obj) must go before if statement in setChar
>
> No, it's OK. You are relying on `setter.invokeExact(obj, c)` to throw NPE
> later...
Yup. This `ensureObj(obj)` call on a Field with no-write access is to ensure
NPE is thrown before IAE consistent with the current behavior.
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]
On Fri, 8 Oct 2021 23:31:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Fix left-over assignment Thanks, Peter. JEP 417 is also updated to reflect this new implementation. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]
On Fri, 8 Oct 2021 23:31:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Fix left-over assignment Change in test/jdk/jdk/jfr/api/consumer/TestHiddenMethod.java looks good. - Marked as reviewed by egahlin (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]
On Fri, 8 Oct 2021 23:31:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Fix left-over assignment I agree that this is in a good state for integration now, though it might be necessary to do some follow-up work to address different performance concerns: - While performance in the `*Const` micros are on the same level as after #5694 there is some added overhead in non-const cases - Cold start overheads leaves a few things to be desired, though the effects we can measure on targeted tests appear to be small in practice on larger apps. Some possible simplifications like using `asSpreader` always is currently unattractive due the added startup overheads. - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: Minor cleanup. Improve javadoc in CallerSensitiveAdapter - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/86d34f48..c25d651a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=11-12 Stats: 95 lines in 3 files changed: 83 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Wed, 13 Oct 2021 18:53:22 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Minor cleanup. Improve javadoc in CallerSensitiveAdapter The JDI problem list and EATest.java changes are approved. test/hotspot/jtreg/ProblemList.txt line 43: > 41: vmTestbase/nsk/jvmti/AttachOnDemand/attach002a/TestDescription.java > 8265795 generic-all > 42: vmTestbase/nsk/jvmti/AttachOnDemand/attach022/TestDescription.java > 8265795 generic-all > 43: > vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java > 8265796 generic-all Approved. test/jdk/com/sun/jdi/EATests.java line 2203: > 2201: // the method depth in debuggee is 11 as it includes all hidden > frames > 2202: // the expected method depth is 6 excluding 5 hidden frames > 2203: testMethodDepth = 11-5; Approved. - Marked as reviewed by cjplummer (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Wed, 13 Oct 2021 18:53:22 GMT, Mandy Chung wrote:
>> This reimplements core reflection with method handles.
>>
>> For `Constructor::newInstance` and `Method::invoke`, the new implementation
>> uses `MethodHandle`. For `Field` accessor, the new implementation uses
>> `VarHandle`.For the first few invocations of one of these reflective
>> methods on a specific reflective object we invoke the corresponding method
>> handle directly. After that we spin a dynamic bytecode stub defined in a
>> hidden class which loads the target `MethodHandle` or `VarHandle` from its
>> class data as a dynamically computed constant. Loading the method handle
>> from a constant allows JIT to inline the method-handle invocation in order
>> to achieve good performance.
>>
>> The VM's native reflection methods are needed during early startup, before
>> the method-handle mechanism is initialized. That happens soon after
>> System::initPhase1 and before System::initPhase2, after which we switch to
>> using method handles exclusively.
>>
>> The core reflection and method handle implementation are updated to handle
>> chained caller-sensitive method calls [1] properly. A caller-sensitive
>> method can define with a caller-sensitive adapter method that will take an
>> additional caller class parameter and the adapter method will be annotated
>> with `@CallerSensitiveAdapter` for better auditing. See the detailed
>> description from [2].
>>
>> Ran tier1-tier8 tests.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8013527
>> [2]
>> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Minor cleanup. Improve javadoc in CallerSensitiveAdapter
Looks a very good simplification. It's great that in the new `poly` benchmarks
the regression is so contained (I was frankly expecting more), and I agree with
the comments (super interesting discussion btw!) that Poly is probably the most
relevant case out there.
I noted that in the static case, Poly does regress for fields - do we know why
instance Poly is better than static Poly? That seems surprising.
src/java.base/share/classes/jdk/internal/reflect/DirectMethodHandleAccessor.java
line 58:
> 56: * Creates a MethodAccessorImpl for a caller-sensitive method.
> 57: */
> 58: static MethodAccessorImpl callerSensitiveMethodAccessor(Method
> method, MethodHandle dmh) {
This method and the one above are identical - they just call `new
DirectMethodHandleAccessor` with same parameters. Is the distinction between
these two factories still relevant? (besides the different asserts)
src/java.base/share/classes/jdk/internal/reflect/DirectMethodHandleAccessor.java
line 86:
> 84: }
> 85:
> 86: private static final int PARAM_COUNT_MASK = 0x00FF;
Is this packing logic required? I get it that it saves footprint - but then you
have to always unmask bits to get the argument count (see invoke and other
places). If you keep this, it might be worth documenting what the bit layout is
supposed to be?
src/java.base/share/classes/jdk/internal/reflect/MethodHandleAccessorFactory.java
line 151:
> 149: var setter = isReadOnly ? null : JLIA.unreflectField(field,
> true);
> 150: Class type = field.getType();
> 151: if (type == Boolean.TYPE) {
dumb question: any reason why `boolean.class` (which is compiled to a LDC) is
not used?
-
Marked as reviewed by mcimadamore (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Thu, 14 Oct 2021 00:10:50 GMT, Maurizio Cimadamore
wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Minor cleanup. Improve javadoc in CallerSensitiveAdapter
>
> src/java.base/share/classes/jdk/internal/reflect/MethodHandleAccessorFactory.java
> line 151:
>
>> 149: var setter = isReadOnly ? null : JLIA.unreflectField(field,
>> true);
>> 150: Class type = field.getType();
>> 151: if (type == Boolean.TYPE) {
>
> dumb question: any reason why `boolean.class` (which is compiled to a LDC) is
> not used?
I only see `boolean.class` compiled to `getstatic Boolean.TYPE`. Is there a
javac flag to compile it to a LDC?
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Wed, 13 Oct 2021 23:49:19 GMT, Maurizio Cimadamore
wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Minor cleanup. Improve javadoc in CallerSensitiveAdapter
>
> src/java.base/share/classes/jdk/internal/reflect/DirectMethodHandleAccessor.java
> line 58:
>
>> 56: * Creates a MethodAccessorImpl for a caller-sensitive method.
>> 57: */
>> 58: static MethodAccessorImpl callerSensitiveMethodAccessor(Method
>> method, MethodHandle dmh) {
>
> This method and the one above are identical - they just call `new
> DirectMethodHandleAccessor` with same parameters. Is the distinction between
> these two factories still relevant? (besides the different asserts)
Good catch! It no longer needs this distinction in this new version. Will
remove it.
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Thu, 14 Oct 2021 00:54:57 GMT, Mandy Chung wrote:
>> src/java.base/share/classes/jdk/internal/reflect/MethodHandleAccessorFactory.java
>> line 151:
>>
>>> 149: var setter = isReadOnly ? null :
>>> JLIA.unreflectField(field, true);
>>> 150: Class type = field.getType();
>>> 151: if (type == Boolean.TYPE) {
>>
>> dumb question: any reason why `boolean.class` (which is compiled to a LDC)
>> is not used?
>
> I only see `boolean.class` compiled to `getstatic Boolean.TYPE`. Is there a
> javac flag to compile it to a LDC?
The LDC bytecode instruction for a class takes a class name not a descriptor as
parameter, so there is no way to encode LDC Z. Valhalla may change that.
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On 14/10/2021 07:05, Rémi Forax wrote:
On Thu, 14 Oct 2021 00:54:57 GMT, Mandy Chung wrote:
src/java.base/share/classes/jdk/internal/reflect/MethodHandleAccessorFactory.java
line 151:
149: var setter = isReadOnly ? null : JLIA.unreflectField(field,
true);
150: Class type = field.getType();
151: if (type == Boolean.TYPE) {
dumb question: any reason why `boolean.class` (which is compiled to a LDC) is
not used?
I only see `boolean.class` compiled to `getstatic Boolean.TYPE`. Is there a
javac flag to compile it to a LDC?
The LDC bytecode instruction for a class takes a class name not a descriptor as
parameter, so there is no way to encode LDC Z. Valhalla may change that.
Remi is right - I got ahead of myself :-)
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Wed, 13 Oct 2021 23:53:17 GMT, Maurizio Cimadamore wrote: >> Mandy Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Minor cleanup. Improve javadoc in CallerSensitiveAdapter > > src/java.base/share/classes/jdk/internal/reflect/DirectMethodHandleAccessor.java > line 86: > >> 84: } >> 85: >> 86: private static final int PARAM_COUNT_MASK = 0x00FF; > > Is this packing logic required? I get it that it saves footprint - but then > you have to always unmask bits to get the argument count (see invoke and > other places). If you keep this, it might be worth documenting what the bit > layout is supposed to be? It's not driven by performance data. It's part of Peter's contribution. I also prefer it without the packing. I propose to keep the parameter count as a separate variable and examine it when there is footprint issue. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Thu, 14 Oct 2021 00:23:29 GMT, Maurizio Cimadamore wrote: > I noted that in the static case, Poly does regress for fields - do we know > why instance Poly is better than static Poly? That seems surprising. I think you're asking why the regression of instance Poly is smaller than that of static Poly? We don't know and Claes will look into it. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Wed, 13 Oct 2021 18:53:22 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Minor cleanup. Improve javadoc in CallerSensitiveAdapter The static Const/Poly/Var is doing better than the instance Const/Poly/Var in the old implementation, which might imply that Unsafe might be slightly faster for static field access than instance field access (I have to dig further.). For the new implementation using MH, the cost of static field access and instance field access looks like similar. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Thu, 14 Oct 2021 19:36:02 GMT, Mandy Chung wrote: >> Mandy Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Minor cleanup. Improve javadoc in CallerSensitiveAdapter > > The static Const/Poly/Var is doing better than the instance Const/Poly/Var in > the old implementation, which might imply that Unsafe might be slightly > faster for static field access than instance field access (I have to dig > further.). > > For the new implementation using MH, the cost of static field access and > instance field access looks like similar. Not sure which results show instance beating static? In both my runs and @mlchung's numbers above static appears slightly faster than instance (note that lower is better here). This seems reasonable since static does not carry a receiver, and a similar relative skew is there in the baseline as well. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Wed, 13 Oct 2021 18:53:22 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Minor cleanup. Improve javadoc in CallerSensitiveAdapter In my runs the relative regression for `instanceFieldPoly` is roughly the same as `staticFieldPoly` (0.61x vs 0.62x). - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Thu, 14 Oct 2021 19:53:46 GMT, Claes Redestad wrote: > In my runs the relative regression for `instanceFieldPoly` is roughly the > same as `staticFieldPoly` (0.61x vs 0.62x). I was looking at this: https://github.com/openjdk/jdk/pull/5027#issuecomment-939185418 (e.g. I didn't run benchmarks myself) baseline ReflectionSpeedBenchmark.instanceFieldPolyavgt 10 67.089 ± 3.288 ns/op JEP 417 ReflectionSpeedBenchmark.instanceFieldPolyavgt 10 98.671 ± 2.015 ns/op - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Thu, 14 Oct 2021 16:05:19 GMT, Mandy Chung wrote: >> src/java.base/share/classes/jdk/internal/reflect/DirectMethodHandleAccessor.java >> line 86: >> >>> 84: } >>> 85: >>> 86: private static final int PARAM_COUNT_MASK = 0x00FF; >> >> Is this packing logic required? I get it that it saves footprint - but then >> you have to always unmask bits to get the argument count (see invoke and >> other places). If you keep this, it might be worth documenting what the bit >> layout is supposed to be? > > It's not driven by performance data. It's part of Peter's contribution. I > also prefer it without the packing. I propose to keep the parameter count > as a separate field and examine it when there is footprint issue. The reason for this packing is (was) ORing the value with a non-zero value so that field never held zero value. When for example an individual value (say paramCount) is used in a separate @Stable field, its value set may include the default value (zero) which is then not optimized by JIT as a constant. This is from @Stable docs: * By extension, any variable (either array or field) which has annotated * as stable is called a stable variable, and its non-null or non-zero * value is called a stable value. ...and: * The HotSpot VM relies on this annotation to promote a non-null (resp., * non-zero) component value to a constant, thereby enabling superior * optimizations of code depending on such a value (such as constant folding). * More specifically, the HotSpot VM will process non-null stable fields (final * or otherwise) in a similar manner to static final fields with respect to * promoting the field's value to a constant. Thus, placing aside the * differences for null/non-null values and arrays, a final stable field is * treated as if it is really final from both the Java language and the HotSpot So now that some of these fields are final and not annotated with @Stable any more but are treated as "trusted final" fields, the question is whether JIT is treating the default (zero, null) values differently or not. If they are treated as static final fields where default value makes no difference, then it's ok to split this multi-valued field into individual fields. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Fri, 15 Oct 2021 15:09:23 GMT, Peter Levart wrote: >> It's not driven by performance data. It's part of Peter's contribution. >> I also prefer it without the packing. I propose to keep the parameter >> count as a separate field and examine it when there is footprint issue. > > The reason for this packing is (was) ORing the value with a non-zero value so > that field never held zero value. When for example an individual value (say > paramCount) is used in a separate @Stable field, its value set may include > the default value (zero) which is then not optimized by JIT as a constant. > This is from @Stable docs: > > * By extension, any variable (either array or field) which has annotated > * as stable is called a stable variable, and its non-null or non-zero > * value is called a stable value. > > ...and: > > * The HotSpot VM relies on this annotation to promote a non-null (resp., > * non-zero) component value to a constant, thereby enabling superior > * optimizations of code depending on such a value (such as constant folding). > * More specifically, the HotSpot VM will process non-null stable fields > (final > * or otherwise) in a similar manner to static final fields with respect to > * promoting the field's value to a constant. Thus, placing aside the > * differences for null/non-null values and arrays, a final stable field is > * treated as if it is really final from both the Java language and the > HotSpot > > So now that some of these fields are final and not annotated with @Stable any > more but are treated as "trusted final" fields, the question is whether JIT > is treating the default (zero, null) values differently or not. If they are > treated as static final fields where default value makes no difference, then > it's ok to split this multi-valued field into individual fields. The compiler team confirms that the zero value only matters for stable fields. For static/trusted non-static final fields, zero value is treated as a constant. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v14]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: Separate paramFlgas into paramCount and flags fields - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/c25d651a..c1bb48fe Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=12-13 Stats: 36 lines in 2 files changed: 9 ins; 13 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v14]
On Mon, 18 Oct 2021 16:10:21 GMT, Mandy Chung wrote:
>> This reimplements core reflection with method handles.
>>
>> For `Constructor::newInstance` and `Method::invoke`, the new implementation
>> uses `MethodHandle`. For `Field` accessor, the new implementation uses
>> `VarHandle`.For the first few invocations of one of these reflective
>> methods on a specific reflective object we invoke the corresponding method
>> handle directly. After that we spin a dynamic bytecode stub defined in a
>> hidden class which loads the target `MethodHandle` or `VarHandle` from its
>> class data as a dynamically computed constant. Loading the method handle
>> from a constant allows JIT to inline the method-handle invocation in order
>> to achieve good performance.
>>
>> The VM's native reflection methods are needed during early startup, before
>> the method-handle mechanism is initialized. That happens soon after
>> System::initPhase1 and before System::initPhase2, after which we switch to
>> using method handles exclusively.
>>
>> The core reflection and method handle implementation are updated to handle
>> chained caller-sensitive method calls [1] properly. A caller-sensitive
>> method can define with a caller-sensitive adapter method that will take an
>> additional caller class parameter and the adapter method will be annotated
>> with `@CallerSensitiveAdapter` for better auditing. See the detailed
>> description from [2].
>>
>> Ran tier1-tier8 tests.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8013527
>> [2]
>> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Separate paramFlgas into paramCount and flags fields
(I've looked at the changed code as someone that is NOT familiar with the
implementation of method handles and reflection. The code did not answer my
question.)
Is it still possible to reflectively invoke methods with 127 * 2 (long) + 1
(int) = 255 parameters?
The following code tests this once using `Method.invoke` and another time it
tries to test it using `MethodHandles.lookup().unreflect()`.
(I'm assuming (!) that the new code in this PR is pretty much "equivalent" to
`MethodHandles.lookup().unreflect()`)
The test using method handles fails with an (expected) IAE.
Question: Can someone confirm that `Method.invoke` will still work with 255
parameters after this PR gets merged?
java.lang.IllegalArgumentException: bad parameter count 256
at
java.base/java.lang.invoke.MethodHandleStatics.newIllegalArgumentException(MethodHandleStatics.java:134)
at
java.base/java.lang.invoke.MethodType.checkSlotCount(MethodType.java:229)
at
java.base/java.lang.invoke.MethodType.insertParameterTypes(MethodType.java:440)
at
java.base/java.lang.invoke.MethodType.appendParameterTypes(MethodType.java:464)
at
java.base/java.lang.invoke.DirectMethodHandle.makePreparedLambdaForm(DirectMethodHandle.java:257)
at
java.base/java.lang.invoke.DirectMethodHandle.preparedLambdaForm(DirectMethodHandle.java:234)
at
java.base/java.lang.invoke.DirectMethodHandle.preparedLambdaForm(DirectMethodHandle.java:219)
at
java.base/java.lang.invoke.DirectMethodHandle.preparedLambdaForm(DirectMethodHandle.java:228)
at
java.base/java.lang.invoke.DirectMethodHandle.make(DirectMethodHandle.java:108)
at
java.base/java.lang.invoke.MethodHandles$Lookup.getDirectMethodCommon(MethodHandles.java:3988)
at
java.base/java.lang.invoke.MethodHandles$Lookup.getDirectMethodNoSecurityManager(MethodHandles.java:3944)
at
java.base/java.lang.invoke.MethodHandles$Lookup.unreflect(MethodHandles.java:3351)
at
ReflectionParameterCountTest.lambda$0(ReflectionParameterCountTest.java:42)
at
org.junit.jupiter.api.AssertDoesNotThrow.assertDoesNotThrow(AssertDoesNotThrow.java:50)
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.reflect.Method;
import org.junit.jupiter.api.Test;
public class ReflectionParameterCountTest {
@Test
public void testReflectionMaxParameterCount() throws Throwable {
Method m = this.getClass().getMethod("f", long.class, long.class,
long.class, long.class, long.class, long.class, long.class, long.class,
long.class, long.class, long.class, long.class, long.class, long.class,
long.class, long.class, long.class, long.class, long.class, long.class,
long.class, long.class, long.class, long.class, long.class, long.class,
long.class, long.class, long.class, long.class, long.class, long.class,
long.class, long.class, long.class, long.clas
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v14]
On Mon, 25 Oct 2021 22:39:47 GMT, intrigus wrote: > Question: Can someone confirm that `Method.invoke` will still work with 255 > parameters after this PR gets merged? Thanks for the test case. For the case when method handles cannot be created due to the arity limit, it can fall back to the VM native reflection support. I have a fix for it. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v15]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 40 commits: - Fall back to the VM native reflection support if method handle cannot be created - fix bug id in test - Merge - Merge branch 'master' of https://github.com/openjdk/jdk into reimplement-method-invoke - Merge branch 'master' of https://github.com/openjdk/jdk into reimplement-method-invoke - Separate paramFlgas into paramCount and flags fields - Minor cleanup. Improve javadoc in CallerSensitiveAdapter - Fix left-over assignment - Remove duplicated microbenchmarks - Avoid pitfall with unwanted inlining in some cases. Also remove boxing/unboxing to focus on the invocation cost - ... and 30 more: https://git.openjdk.java.net/jdk/compare/97d3280e...64738bb2 - Changes: https://git.openjdk.java.net/jdk/pull/5027/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=14 Stats: 6409 lines in 78 files changed: 5864 ins; 317 del; 228 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v15]
On Tue, 26 Oct 2021 16:35:34 GMT, Mandy Chung wrote:
>> This reimplements core reflection with method handles.
>>
>> For `Constructor::newInstance` and `Method::invoke`, the new implementation
>> uses `MethodHandle`. For `Field` accessor, the new implementation uses
>> `VarHandle`.For the first few invocations of one of these reflective
>> methods on a specific reflective object we invoke the corresponding method
>> handle directly. After that we spin a dynamic bytecode stub defined in a
>> hidden class which loads the target `MethodHandle` or `VarHandle` from its
>> class data as a dynamically computed constant. Loading the method handle
>> from a constant allows JIT to inline the method-handle invocation in order
>> to achieve good performance.
>>
>> The VM's native reflection methods are needed during early startup, before
>> the method-handle mechanism is initialized. That happens soon after
>> System::initPhase1 and before System::initPhase2, after which we switch to
>> using method handles exclusively.
>>
>> The core reflection and method handle implementation are updated to handle
>> chained caller-sensitive method calls [1] properly. A caller-sensitive
>> method can define with a caller-sensitive adapter method that will take an
>> additional caller class parameter and the adapter method will be annotated
>> with `@CallerSensitiveAdapter` for better auditing. See the detailed
>> description from [2].
>>
>> Ran tier1-tier8 tests.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8013527
>> [2]
>> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430
>
> Mandy Chung has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 40 commits:
>
> - Fall back to the VM native reflection support if method handle cannot be
> created
> - fix bug id in test
> - Merge
> - Merge branch 'master' of https://github.com/openjdk/jdk into
> reimplement-method-invoke
> - Merge branch 'master' of https://github.com/openjdk/jdk into
> reimplement-method-invoke
> - Separate paramFlgas into paramCount and flags fields
> - Minor cleanup. Improve javadoc in CallerSensitiveAdapter
> - Fix left-over assignment
> - Remove duplicated microbenchmarks
> - Avoid pitfall with unwanted inlining in some cases. Also remove
> boxing/unboxing to focus on the invocation cost
> - ... and 30 more:
> https://git.openjdk.java.net/jdk/compare/97d3280e...64738bb2
src/java.base/share/classes/jdk/internal/reflect/AccessorUtils.java line 34:
> 32: */
> 33: public class AccessorUtils {
> 34: static boolean isIllegalArgument(Class accessorType,
> RuntimeException e) {
It might be useful to add a method description. In isolation, it's not
immediately clear what it does.
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v15]
On Wed, 27 Oct 2021 14:08:05 GMT, Alan Bateman wrote:
>> Mandy Chung has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 40 commits:
>>
>> - Fall back to the VM native reflection support if method handle cannot be
>> created
>> - fix bug id in test
>> - Merge
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> reimplement-method-invoke
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> reimplement-method-invoke
>> - Separate paramFlgas into paramCount and flags fields
>> - Minor cleanup. Improve javadoc in CallerSensitiveAdapter
>> - Fix left-over assignment
>> - Remove duplicated microbenchmarks
>> - Avoid pitfall with unwanted inlining in some cases. Also remove
>> boxing/unboxing to focus on the invocation cost
>> - ... and 30 more:
>> https://git.openjdk.java.net/jdk/compare/97d3280e...64738bb2
>
> src/java.base/share/classes/jdk/internal/reflect/AccessorUtils.java line 34:
>
>> 32: */
>> 33: public class AccessorUtils {
>> 34: static boolean isIllegalArgument(Class accessorType,
>> RuntimeException e) {
>
> It might be useful to add a method description. In isolation, it's not
> immediately clear what it does.
I agree that'd be helpful. I'll add the javadoc.
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 43 commits: - fix copyright header and typo - improve documentation of AccessorUtils - Merge branch 'master' of https://github.com/openjdk/jdk into reimplement-method-invoke - Fall back to the VM native reflection support if method handle cannot be created - fix bug id in test - Merge - Merge branch 'master' of https://github.com/openjdk/jdk into reimplement-method-invoke - Merge branch 'master' of https://github.com/openjdk/jdk into reimplement-method-invoke - Separate paramFlgas into paramCount and flags fields - Minor cleanup. Improve javadoc in CallerSensitiveAdapter - ... and 33 more: https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b - Changes: https://git.openjdk.java.net/jdk/pull/5027/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5027&range=15 Stats: 6436 lines in 78 files changed: 5891 ins; 317 del; 228 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Wed, 27 Oct 2021 20:16:54 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 43 commits: > > - fix copyright header and typo > - improve documentation of AccessorUtils > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Fall back to the VM native reflection support if method handle cannot be > created > - fix bug id in test > - Merge > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Separate paramFlgas into paramCount and flags fields > - Minor cleanup. Improve javadoc in CallerSensitiveAdapter > - ... and 33 more: > https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b Thanks all for the review. JEP 416 is now target to JDK 18. I'll integrate this PR today. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Wed, 27 Oct 2021 20:16:54 GMT, Mandy Chung wrote:
>> This reimplements core reflection with method handles.
>>
>> For `Constructor::newInstance` and `Method::invoke`, the new implementation
>> uses `MethodHandle`. For `Field` accessor, the new implementation uses
>> `VarHandle`.For the first few invocations of one of these reflective
>> methods on a specific reflective object we invoke the corresponding method
>> handle directly. After that we spin a dynamic bytecode stub defined in a
>> hidden class which loads the target `MethodHandle` or `VarHandle` from its
>> class data as a dynamically computed constant. Loading the method handle
>> from a constant allows JIT to inline the method-handle invocation in order
>> to achieve good performance.
>>
>> The VM's native reflection methods are needed during early startup, before
>> the method-handle mechanism is initialized. That happens soon after
>> System::initPhase1 and before System::initPhase2, after which we switch to
>> using method handles exclusively.
>>
>> The core reflection and method handle implementation are updated to handle
>> chained caller-sensitive method calls [1] properly. A caller-sensitive
>> method can define with a caller-sensitive adapter method that will take an
>> additional caller class parameter and the adapter method will be annotated
>> with `@CallerSensitiveAdapter` for better auditing. See the detailed
>> description from [2].
>>
>> Ran tier1-tier8 tests.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8013527
>> [2]
>> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430
>
> Mandy Chung has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 43 commits:
>
> - fix copyright header and typo
> - improve documentation of AccessorUtils
> - Merge branch 'master' of https://github.com/openjdk/jdk into
> reimplement-method-invoke
> - Fall back to the VM native reflection support if method handle cannot be
> created
> - fix bug id in test
> - Merge
> - Merge branch 'master' of https://github.com/openjdk/jdk into
> reimplement-method-invoke
> - Merge branch 'master' of https://github.com/openjdk/jdk into
> reimplement-method-invoke
> - Separate paramFlgas into paramCount and flags fields
> - Minor cleanup. Improve javadoc in CallerSensitiveAdapter
> - ... and 33 more:
> https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b
Good work.
src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
683:
> 681: } else if (val.equals("fields")) {
> 682: useDirectMethodHandle = FIELD_MH_ACCESSOR;
> 683: }
Happy to see that property can be used to control both.
-
Marked as reviewed by alanb (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Wed, 27 Oct 2021 20:16:54 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 43 commits: > > - fix copyright header and typo > - improve documentation of AccessorUtils > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Fall back to the VM native reflection support if method handle cannot be > created > - fix bug id in test > - Merge > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Separate paramFlgas into paramCount and flags fields > - Minor cleanup. Improve javadoc in CallerSensitiveAdapter > - ... and 33 more: > https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b Hi, Apache Wicket tests started failing with JDK 18 b23 and I think it is caused by this PR. I've sent an email to Rory O'Donnell and his team because we participate at https://wiki.openjdk.java.net/display/quality/Quality+Outreach The email with the explanation of the problem could be seen at https://markmail.org/message/o3gw72bwsfrpaf2n - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Sat, 13 Nov 2021 23:56:20 GMT, Martin Grigorov wrote: >> Mandy Chung has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 43 commits: >> >> - fix copyright header and typo >> - improve documentation of AccessorUtils >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Fall back to the VM native reflection support if method handle cannot be >> created >> - fix bug id in test >> - Merge >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Separate paramFlgas into paramCount and flags fields >> - Minor cleanup. Improve javadoc in CallerSensitiveAdapter >> - ... and 33 more: >> https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b > > Hi, > > Apache Wicket tests started failing with JDK 18 b23 and I think it is caused > by this PR. > I've sent an email to Rory O'Donnell and his team because we participate at > https://wiki.openjdk.java.net/display/quality/Quality+Outreach > The email with the explanation of the problem could be seen at > https://markmail.org/message/o3gw72bwsfrpaf2n @martin-g mutating static final fields with reflection `setAccessible(true)` is an ugly hack with partially undefined behavior that can lead to all manners of bugs.. but I think this is an unintentional behavior change. @mlchung should verify, but it looks like the `MethodHandles.unreflect` API used internally in the new implementation is slightly stricter and ignores the `setAccessible(true)` for the trusted finality check. This added strictness is established behavior for the public `MethodHandles.unreflect` API point, which seem perfectly fine there (the MH API is strictly adhering to regular java access rules). For this new reflection implementation we should probably make an exception to that strictness to be perfectly backwards compatible. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On 14/11/2021 00:59, Claes Redestad wrote: : @martin-g mutating static final fields with reflection `setAccessible(true)` is an ugly hack with partially undefined behavior that can lead to all manners of bugs.. but I think this is an unintentional behavior change. @mlchung should verify, but it looks like the `MethodHandles.unreflect` API used internally in the new implementation is slightly stricter and ignores the `setAccessible(true)` for the trusted finality check. This added strictness is established behavior for the public `MethodHandles.unreflect` API point, which seem perfectly fine there (the MH API is strictly adhering to regular java access rules). For this new reflection implementation we should probably make an exception to that strictness to be perfectly backwards compatible. If I read this correctly, a wicked Wicket test is making use of a private method in java.lang.Class so it can hack jlr.Field and change its internal "modifiers" field. I don't think the JDK should be expected to keep crazy hacks like this working from release to release. Are there many Wicket tests trying to modify static field fields? Have you looked at using an agent to drop the final modifier from these fields when loading the classes? -Alan
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Wed, 27 Oct 2021 20:16:54 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 43 commits: > > - fix copyright header and typo > - improve documentation of AccessorUtils > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Fall back to the VM native reflection support if method handle cannot be > created > - fix bug id in test > - Merge > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Separate paramFlgas into paramCount and flags fields > - Minor cleanup. Improve javadoc in CallerSensitiveAdapter > - ... and 33 more: > https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b Thank you for the comments! There is no need to be harsh though! We just made use of old Reflection APIs. They were working for many years and according to StackOverflow Apache Wicket is not the only one doing this! In our case it is done in a test case, so it is easy to rework it. > Are there many Wicket tests trying to modify static field fields? No! We have just one failing test with JDK 18 b23. In someone else's case it might be needed to "hack" a third party library. As a good citizen I'm reporting it back to you! - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Sun, 14 Nov 2021 19:54:50 GMT, Martin Grigorov wrote: >> Mandy Chung has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 43 commits: >> >> - fix copyright header and typo >> - improve documentation of AccessorUtils >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Fall back to the VM native reflection support if method handle cannot be >> created >> - fix bug id in test >> - Merge >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Separate paramFlgas into paramCount and flags fields >> - Minor cleanup. Improve javadoc in CallerSensitiveAdapter >> - ... and 33 more: >> https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b > > Thank you for the comments! > There is no need to be harsh though! We just made use of old Reflection APIs. > They were working for many years and according to StackOverflow Apache Wicket > is not the only one doing this! > In our case it is done in a test case, so it is easy to rework it. > >> Are there many Wicket tests trying to modify static field fields? > > No! We have just one failing test with JDK 18 b23. > > In someone else's case it might be needed to "hack" a third party library. > > As a good citizen I'm reporting it back to you! @martin-g I wasn't making any judgement. To qualify why this is not something I think anyone should do: poking into reflection internals to make a static final field mutable opens you up to situations where some readers still see the old value. An optimizing compiler (JIT or AOT) may treat the value as constant and propagates it. Future JVM features might decide to be even more aggressive, and trust these fields even more completely. While it's unspecified what should happen after overwriting a static final field with reflection, it does work if you're lucky. The one-off use in Wicket for a test seem benign enough. Alan: changing `Field.modifiers` still works, but dropping the final modifier is not enough for this to work in the new impl. It won't be hard to adapt to the new world. Users who relies on this today could for example opt-out of the new MH-based impl using `-Djdk.reflect.useDirectMethodHandle=false` and get the old behavior. I've checked using a minimal reproducer I extracted from the Wicket sources that this works. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On 14/11/2021 22:56, Claes Redestad wrote: : Alan: changing `Field.modifiers` still works, but dropping the final modifier is not enough for this to work in the new impl. It won't be hard to adapt to the new world. Users who relies on this today could for example opt-out of the new MH-based impl using `-Djdk.reflect.useDirectMethodHandle=false` and get the old behavior. I've checked using a minimal reproducer I extracted from the Wicket sources that this works. Sure, but I don't think that would be enough as Wicket would also need to open java.lang and java.lang.reflect to allow it continue to access private members of Class and Field. I assume the test started emitting "Illegal reflective access ..." warnings in JDK 9 and it stopped working in JDK 16, and somewhere along the line the maintainers must have added --add-opens to get it to work. It's just not tenable, hopefully the project will find a way to re-write that test. -Alan
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Wed, 27 Oct 2021 20:16:54 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 43 commits: > > - fix copyright header and typo > - improve documentation of AccessorUtils > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Fall back to the VM native reflection support if method handle cannot be > created > - fix bug id in test > - Merge > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Separate paramFlgas into paramCount and flags fields > - Minor cleanup. Improve javadoc in CallerSensitiveAdapter > - ... and 33 more: > https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b I have already fixed our build with https://github.com/apache/wicket/commit/191de985e22b9e0801d5783fbcfa151a25d29ec8 and https://github.com/apache/wicket/commit/128125f25c33a4d886386291f24ffe2d195007ac Depending on your decision whether to make it possible again to drop `final` for `static` fields I will either revert these changes or not. The main purpose of my report is to let you know about this "regression". - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
Hi Alan, On 15/11/2021 5:11 pm, Alan Bateman wrote: On 14/11/2021 22:56, Claes Redestad wrote: : Alan: changing `Field.modifiers` still works, but dropping the final modifier is not enough for this to work in the new impl. It won't be hard to adapt to the new world. Users who relies on this today could for example opt-out of the new MH-based impl using `-Djdk.reflect.useDirectMethodHandle=false` and get the old behavior. I've checked using a minimal reproducer I extracted from the Wicket sources that this works. Sure, but I don't think that would be enough as Wicket would also need to open java.lang and java.lang.reflect to allow it continue to access private members of Class and Field. I think there may be a misunderstanding here, AFAICS they are using reflection to remove the final-ness of a field in their own classes, not modifying anything in Class or Field. Cheers, David - I assume the test started emitting "Illegal reflective access ..." warnings in JDK 9 and it stopped working in JDK 16, and somewhere along the line the maintainers must have added --add-opens to get it to work. It's just not tenable, hopefully the project will find a way to re-write that test. -Alan
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On 15/11/2021 09:48, David Holmes wrote: I think there may be a misunderstanding here, AFAICS they are using reflection to remove the final-ness of a field in their own classes, not modifying anything in Class or Field. That's the outcome. To get there, they call a private method getDeclaredFields0 on j.l.Class and also change the value of the private "modifiers" field in jlr.Field. It's just not tenable to hack into private members like this. Martin seems to have done the right thing and removed it. -Alan.
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On 15/11/2021 8:14 pm, Alan Bateman wrote: On 15/11/2021 09:48, David Holmes wrote: I think there may be a misunderstanding here, AFAICS they are using reflection to remove the final-ness of a field in their own classes, not modifying anything in Class or Field. That's the outcome. To get there, they call a private method getDeclaredFields0 on j.l.Class and also change the value of the private "modifiers" field in jlr.Field. It's just not tenable to hack into private members like this. Martin seems to have done the right thing and removed it. Apologies - the misunderstanding was mine. David -Alan.
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Mon, 15 Nov 2021 07:33:00 GMT, Martin Grigorov wrote: >> Mandy Chung has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 43 commits: >> >> - fix copyright header and typo >> - improve documentation of AccessorUtils >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Fall back to the VM native reflection support if method handle cannot be >> created >> - fix bug id in test >> - Merge >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Separate paramFlgas into paramCount and flags fields >> - Minor cleanup. Improve javadoc in CallerSensitiveAdapter >> - ... and 33 more: >> https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b > > I have already fixed our build with > https://github.com/apache/wicket/commit/191de985e22b9e0801d5783fbcfa151a25d29ec8 > and > https://github.com/apache/wicket/commit/128125f25c33a4d886386291f24ffe2d195007ac > Depending on your decision whether to make it possible again to drop `final` > for `static` fields I will either revert these changes or not. > The main purpose of my report is to let you know about this "regression". @martin-g Thanks for reporting this. Appreciated. JEP 416 makes `java.lang.reflect` classes *trusted* that reveals this another attempt to change the value of the private final `Field::modifiers` field via reflection. Since JDK 12 after https://bugs.openjdk.java.net/browse/JDK-8210496, all security-sensitive fields in `Field` and other java.lang.reflect classes are filtered from reflective access. In other words, since Java 12, `Field::modifiers` cannot be found through reflection and hence it can't be used to change the value of the modifiers of a field. The implementation of JEP 416 hardens the restriction further. To drop `final` from the modifiers, one should look into using an instrumentation agent, as Alan suggests. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Wed, 27 Oct 2021 20:16:54 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 43 commits: > > - fix copyright header and typo > - improve documentation of AccessorUtils > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Fall back to the VM native reflection support if method handle cannot be > created > - fix bug id in test > - Merge > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Separate paramFlgas into paramCount and flags fields > - Minor cleanup. Improve javadoc in CallerSensitiveAdapter > - ... and 33 more: > https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b I further looked at the specification and implementation of `Field::set` and adding `java.lang.reflect` to the trusted final field package list does not relate to this issue. I was confused with the `isTrustedFinalField` check which is supposed to check if the given `Field` object has read-only access. A `Field` object has write access if and only if - setAccessible(true) has succeeded for this Field object; - the field is non-static; and - the field's declaring class is not a hidden class; and - the field's declaring class is not a record class. The hack dropping `final` from a static final `Field` make a `Field` object which has only read-only access to get write access. With JEP 416, this hack no longer works because the read-only access is set according to the original field modifiers but not the `Field` object. - PR: https://git.openjdk.java.net/jdk/pull/5027
