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 [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 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 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 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 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]
> 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
