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 [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 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 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=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: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=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 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 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 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 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 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: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 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=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 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=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]
> 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=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=5027=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5027=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
