RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Boris, thanks, I've updated the BUFFER_SIZE in place. Seems like all platform implementations have been reviewed. So I'll push this version if there are no objections. Thanks everyone for reviewing! Best regards, Martin > -Original Message- > From: Boris Ulasevich > Sent: Freitag, 26. Juli 2019 18:18 > To: Doerr, Martin > Cc: hotspot-runtime-...@openjdk.java.net; serviceability- > d...@openjdk.java.net > Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access > event requests at runtime > > Hi Martin, > > The webrev.02 change works good if we increase BUFFER_SIZE. Current > change gives "BUFFER_SIZE too small" assertion. I propose to change > BUFFER_SIZE value to 120, it works Ok then. > > glad to help you, > regards, > Boris > > On 26.07.2019 16:59, Doerr, Martin wrote: > > Hi Boris, > > > > thank you very much for testing. > > > > Unfortunately, arm 32 was also affected by the issue Erik has found for > aarch64: > > We need a little stronger memory barriers to support accessing volatile > fields with correct ordering semantics. > > > > I've updated that in the current webrev already: > > > http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.02/ > > > > I'm using > membar(MacroAssembler::Membar_mask_bits(MacroAssembler::LoadLoad > | MacroAssembler::LoadStore), Rtmp2), now. > > I've already used a cross build to check that it compiles, but I haven't > > run it. > > I believe this membar doesn't have a significant performance impact. > > > > Would be great if you could take a look and test that, too. > > > > Thanks and best regards, > > Martin > > > > > >> -Original Message----- > >> From: Boris Ulasevich > >> Sent: Freitag, 26. Juli 2019 12:50 > >> To: Doerr, Martin > >> Cc: hotspot-runtime-...@openjdk.java.net; serviceability- > >> d...@openjdk.java.net > >> Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field > access > >> event requests at runtime > >> > >> Hi Martin, > >> > >> Your change works Ok on arm32 with the minor correction. See the patch > >> attached. > >> > >> thanks, > >> Boris > >> > >> On 16.07.2019 16:31, Doerr, Martin wrote: > >>> Hi, > >>> > >>> the current implementation of FastJNIAccessors ignores the flag - > >> XX:+UseFastJNIAccessors when the JVMTI capability > >> "can_post_field_access" is enabled. > >>> This is an unnecessary restriction which makes field accesses > >> (GetField) from native code slower when a JVMTI agent is > attached > >> which enables this capability. > >>> A better implementation would check at runtime if an agent actually > wants > >> to receive field access events. > >>> > >>> Note that the bytecode interpreter already uses this better > >> implementation by checking if field access watch events were requested > >> (JvmtiExport::_field_access_count != 0). > >>> > >>> I have implemented such a runtime check on all platforms which > currently > >> support FastJNIAccessors. > >>> > >>> My new jtreg test runtime/jni/FastGetField/FastGetField.java contains a > >> micro benchmark: > >>> test- > >> > support/jtreg_test_hotspot_jtreg_runtime_jni_FastGetField/runtime/jni/Fa > >> stGetField/FastGetField.jtr > >>> shows the duration of 1 iterations with and without > >> UseFastJNIAccessors (JVMTI agent gets attached in both runs). > >>> My Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz needed 4.7ms with > >> FastJNIAccessors and 11.2ms without it. > >>> > >>> Webrev: > >>> > >> > http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.00/ > >>> > >>> We have run the test on 64 bit x86 platforms, SPARC and aarch64. > >>> (FastJNIAccessors are not yet available on PPC64 and s390. I'll contribute > >> them later.) > >>> My webrev contains 32 bit implementations for x86 and arm, but > >> completely untested. It'd be great if somebody could volunteer to review > >> and test these platforms. > >>> > >>> Please review. > >>> > >>> Best regards, > >>> Martin > >>>
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Martin, The webrev.02 change works good if we increase BUFFER_SIZE. Current change gives "BUFFER_SIZE too small" assertion. I propose to change BUFFER_SIZE value to 120, it works Ok then. glad to help you, regards, Boris On 26.07.2019 16:59, Doerr, Martin wrote: Hi Boris, thank you very much for testing. Unfortunately, arm 32 was also affected by the issue Erik has found for aarch64: We need a little stronger memory barriers to support accessing volatile fields with correct ordering semantics. I've updated that in the current webrev already: http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.02/ I'm using membar(MacroAssembler::Membar_mask_bits(MacroAssembler::LoadLoad | MacroAssembler::LoadStore), Rtmp2), now. I've already used a cross build to check that it compiles, but I haven't run it. I believe this membar doesn't have a significant performance impact. Would be great if you could take a look and test that, too. Thanks and best regards, Martin -Original Message- From: Boris Ulasevich Sent: Freitag, 26. Juli 2019 12:50 To: Doerr, Martin Cc: hotspot-runtime-...@openjdk.java.net; serviceability- d...@openjdk.java.net Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime Hi Martin, Your change works Ok on arm32 with the minor correction. See the patch attached. thanks, Boris On 16.07.2019 16:31, Doerr, Martin wrote: Hi, the current implementation of FastJNIAccessors ignores the flag - XX:+UseFastJNIAccessors when the JVMTI capability "can_post_field_access" is enabled. This is an unnecessary restriction which makes field accesses (GetField) from native code slower when a JVMTI agent is attached which enables this capability. A better implementation would check at runtime if an agent actually wants to receive field access events. Note that the bytecode interpreter already uses this better implementation by checking if field access watch events were requested (JvmtiExport::_field_access_count != 0). I have implemented such a runtime check on all platforms which currently support FastJNIAccessors. My new jtreg test runtime/jni/FastGetField/FastGetField.java contains a micro benchmark: test- support/jtreg_test_hotspot_jtreg_runtime_jni_FastGetField/runtime/jni/Fa stGetField/FastGetField.jtr shows the duration of 1 iterations with and without UseFastJNIAccessors (JVMTI agent gets attached in both runs). My Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz needed 4.7ms with FastJNIAccessors and 11.2ms without it. Webrev: http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.00/ We have run the test on 64 bit x86 platforms, SPARC and aarch64. (FastJNIAccessors are not yet available on PPC64 and s390. I'll contribute them later.) My webrev contains 32 bit implementations for x86 and arm, but completely untested. It'd be great if somebody could volunteer to review and test these platforms. Please review. Best regards, Martin
RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Boris, thank you very much for testing. Unfortunately, arm 32 was also affected by the issue Erik has found for aarch64: We need a little stronger memory barriers to support accessing volatile fields with correct ordering semantics. I've updated that in the current webrev already: http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.02/ I'm using membar(MacroAssembler::Membar_mask_bits(MacroAssembler::LoadLoad | MacroAssembler::LoadStore), Rtmp2), now. I've already used a cross build to check that it compiles, but I haven't run it. I believe this membar doesn't have a significant performance impact. Would be great if you could take a look and test that, too. Thanks and best regards, Martin > -Original Message- > From: Boris Ulasevich > Sent: Freitag, 26. Juli 2019 12:50 > To: Doerr, Martin > Cc: hotspot-runtime-...@openjdk.java.net; serviceability- > d...@openjdk.java.net > Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access > event requests at runtime > > Hi Martin, > > Your change works Ok on arm32 with the minor correction. See the patch > attached. > > thanks, > Boris > > On 16.07.2019 16:31, Doerr, Martin wrote: > > Hi, > > > > the current implementation of FastJNIAccessors ignores the flag - > XX:+UseFastJNIAccessors when the JVMTI capability > "can_post_field_access" is enabled. > > This is an unnecessary restriction which makes field accesses > (GetField) from native code slower when a JVMTI agent is attached > which enables this capability. > > A better implementation would check at runtime if an agent actually wants > to receive field access events. > > > > Note that the bytecode interpreter already uses this better > implementation by checking if field access watch events were requested > (JvmtiExport::_field_access_count != 0). > > > > I have implemented such a runtime check on all platforms which currently > support FastJNIAccessors. > > > > My new jtreg test runtime/jni/FastGetField/FastGetField.java contains a > micro benchmark: > > test- > support/jtreg_test_hotspot_jtreg_runtime_jni_FastGetField/runtime/jni/Fa > stGetField/FastGetField.jtr > > shows the duration of 1 iterations with and without > UseFastJNIAccessors (JVMTI agent gets attached in both runs). > > My Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz needed 4.7ms with > FastJNIAccessors and 11.2ms without it. > > > > Webrev: > > > http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.00/ > > > > We have run the test on 64 bit x86 platforms, SPARC and aarch64. > > (FastJNIAccessors are not yet available on PPC64 and s390. I'll contribute > them later.) > > My webrev contains 32 bit implementations for x86 and arm, but > completely untested. It'd be great if somebody could volunteer to review > and test these platforms. > > > > Please review. > > > > Best regards, > > Martin > >
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Martin,
Your change works Ok on arm32 with the minor correction. See the patch
attached.
thanks,
Boris
On 16.07.2019 16:31, Doerr, Martin wrote:
Hi,
the current implementation of FastJNIAccessors ignores the flag -XX:+UseFastJNIAccessors
when the JVMTI capability "can_post_field_access" is enabled.
This is an unnecessary restriction which makes field accesses (GetField)
from native code slower when a JVMTI agent is attached which enables this capability.
A better implementation would check at runtime if an agent actually wants to
receive field access events.
Note that the bytecode interpreter already uses this better implementation by
checking if field access watch events were requested
(JvmtiExport::_field_access_count != 0).
I have implemented such a runtime check on all platforms which currently
support FastJNIAccessors.
My new jtreg test runtime/jni/FastGetField/FastGetField.java contains a micro
benchmark:
test-support/jtreg_test_hotspot_jtreg_runtime_jni_FastGetField/runtime/jni/FastGetField/FastGetField.jtr
shows the duration of 1 iterations with and without UseFastJNIAccessors
(JVMTI agent gets attached in both runs).
My Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz needed 4.7ms with FastJNIAccessors
and 11.2ms without it.
Webrev:
http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.00/
We have run the test on 64 bit x86 platforms, SPARC and aarch64.
(FastJNIAccessors are not yet available on PPC64 and s390. I'll contribute them
later.)
My webrev contains 32 bit implementations for x86 and arm, but completely
untested. It'd be great if somebody could volunteer to review and test these
platforms.
Please review.
Best regards,
Martin
--- a/src/hotspot/cpu/arm/jniFastGetField_arm.cpp 2019-07-26 13:29:34.569851539 +0300
+++ b/src/hotspot/cpu/arm/jniFastGetField_arm.cpp 2019-07-26 13:31:34.441884864 +0300
@@ -32,7 +32,7 @@
#define __ masm->
-#define BUFFER_SIZE 96
+#define BUFFER_SIZE 120
address JNI_FastGetField::generate_fast_get_int_field0(BasicType type) {
const char* name = NULL;
@@ -114,7 +114,7 @@
if (JvmtiExport::can_post_field_access()) {
// Using barrier to order wrt. JVMTI check and load of result.
-__ membar(Assembler::LoadLoad, Rtmp1);
+__ membar(MacroAssembler::LoadLoad, Rtmp1);
// Check to see if a field access watch has been set before we
// take the fast path.
@@ -191,7 +191,7 @@
if (JvmtiExport::can_post_field_access()) {
// Order JVMTI check and load of result wrt. succeeding check.
-__ membar(Assembler::LoadLoad, Rtmp2);
+__ membar(MacroAssembler::LoadLoad, Rtmp2);
__ ldr_s32(Rsafept_cnt2, Address(Rsafepoint_counter_addr));
} else {
// Address dependency restricts memory access ordering. It's cheaper than explicit LoadLoad barrier
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Martin, Looks good for me. Thanks for cleaning up this code! /Erik > On 23 Jul 2019, at 19:50, Doerr, Martin wrote: > > Hi Erik, > > adding Andrew and Aleksey. > >> The new webrev looks good. > Thanks. > >> Note though the following though... it looks like the AArch64 code >> doesn't do appropriate fencing if the field is volatile. > I agree. I was not aware of JDK-8179954 > (https://bugs.openjdk.java.net/browse/JDK-8179954). > > My new aarch64 proposal: > http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.02/ > > Note: All platforms were tested except arm (32 bit). I could also return > (address)-1 if JvmtiExport::can_post_field_access() in case nobody wants this > for arm. > > Best regards, > Martin >
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Martin, No further comments from me. I'm obviously not knowledgeable enough to review any of the assembler changes. Thanks, David On 23/07/2019 8:29 pm, Doerr, Martin wrote: Hi David and Erik, thank you for reviewing and for your very valuable feedback. 1) In the x86_64 assembly, you can combine the movl; testl; into a single test instruction with one memory operand to the counter, and one immediate zero. Thanks for the hint. I'm using cmp32 in my new webrev. 2) If libjvm.so maps in far away, then the movl taking an ExternalAddress, will actually scratch rscratch1, which is r10. Good catch! I've exchanged registers and added assert_different_registers. I was secretly hoping to never have to touch fast JNI getfield again, because it is so shady, and the odd cases are very hard to test, making it so easy to mess up. The ForceUnreachable JVM flag might be useful in checking if a solution works also when rscratch1 gets clobbered when referencing JVM symbols that are now "far away". I've also changed the test to run with -XX:+ForceUnreachable and -XX:+SafepointALot to hit more corner cases. But as you explained, the test would normally not notice the destroyed counter and just execute the slow path. The subtle issue of referencing JVM symbols that can be far away, suddenly clobbering r10, has bitten us many times. Perhaps it should be made more explicit somehow. It would be possible to explicitly kill r10 in all such assembler instructions in the dbg build, but that'd come with an overhead. But that's a separate issue. Agreed. Also, I noticed that the counter that we are checking if it has changed, is a 32 bit signed integer. They can actually wrap around, which is undefined behaviour at best, and will make these tests fail in the worst case. When we don't want counters to overflow, we use 64 bit integers. We could also make it unsigned to get defined behavior, but that's out of scope here. New webrev: http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.01/ Best regards, Martin
RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Erik, adding Andrew and Aleksey. > The new webrev looks good. Thanks. > Note though the following though... it looks like the AArch64 code > doesn't do appropriate fencing if the field is volatile. I agree. I was not aware of JDK-8179954 (https://bugs.openjdk.java.net/browse/JDK-8179954). My new aarch64 proposal: http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.02/ Note: All platforms were tested except arm (32 bit). I could also return (address)-1 if JvmtiExport::can_post_field_access() in case nobody wants this for arm. Best regards, Martin
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Martin, The new webrev looks good. Note though the following though... it looks like the AArch64 code doesn't do appropriate fencing if the field is volatile. The normal JNI accessor goes through thread transitions causing the following semantics: fence() load fence() Which is more than enough for a volatile field load. However, with JNI fast get field... it is insufficient. Thanks, /Erik On 2019-07-23 12:29, Doerr, Martin wrote: Hi David and Erik, thank you for reviewing and for your very valuable feedback. 1) In the x86_64 assembly, you can combine the movl; testl; into a single test instruction with one memory operand to the counter, and one immediate zero. Thanks for the hint. I'm using cmp32 in my new webrev. 2) If libjvm.so maps in far away, then the movl taking an ExternalAddress, will actually scratch rscratch1, which is r10. Good catch! I've exchanged registers and added assert_different_registers. I was secretly hoping to never have to touch fast JNI getfield again, because it is so shady, and the odd cases are very hard to test, making it so easy to mess up. The ForceUnreachable JVM flag might be useful in checking if a solution works also when rscratch1 gets clobbered when referencing JVM symbols that are now "far away". I've also changed the test to run with -XX:+ForceUnreachable and -XX:+SafepointALot to hit more corner cases. But as you explained, the test would normally not notice the destroyed counter and just execute the slow path. The subtle issue of referencing JVM symbols that can be far away, suddenly clobbering r10, has bitten us many times. Perhaps it should be made more explicit somehow. It would be possible to explicitly kill r10 in all such assembler instructions in the dbg build, but that'd come with an overhead. But that's a separate issue. Agreed. Also, I noticed that the counter that we are checking if it has changed, is a 32 bit signed integer. They can actually wrap around, which is undefined behaviour at best, and will make these tests fail in the worst case. When we don't want counters to overflow, we use 64 bit integers. We could also make it unsigned to get defined behavior, but that's out of scope here. New webrev: http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.01/ Best regards, Martin
RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi David and Erik, thank you for reviewing and for your very valuable feedback. > 1) In the x86_64 assembly, you can combine the movl; testl; into a single > test instruction with one memory operand to the counter, and one > immediate zero. Thanks for the hint. I'm using cmp32 in my new webrev. > 2) If libjvm.so maps in far away, then the movl taking an ExternalAddress, > will actually scratch rscratch1, which is r10. Good catch! I've exchanged registers and added assert_different_registers. > I was secretly hoping to never have to touch fast JNI getfield again, > because it is so shady, and the odd cases are very hard to test, making it so > easy to mess up. The ForceUnreachable JVM flag might be useful in checking > if a solution works also when rscratch1 gets clobbered when referencing JVM > symbols that are now "far away". I've also changed the test to run with -XX:+ForceUnreachable and -XX:+SafepointALot to hit more corner cases. But as you explained, the test would normally not notice the destroyed counter and just execute the slow path. > The subtle issue of referencing JVM symbols that can be far away, > suddenly clobbering r10, has bitten us many times. Perhaps it should be > made more explicit somehow. It would be possible to explicitly kill r10 in all such assembler instructions in the dbg build, but that'd come with an overhead. > But that's a separate issue. Agreed. > Also, I noticed that the counter that we are checking if it has changed, is a > 32 bit signed integer. They can actually wrap around, which is undefined > behaviour at best, and will make these tests fail in the worst case. When we > don't want counters to overflow, we use 64 bit integers. We could also make it unsigned to get defined behavior, but that's out of scope here. New webrev: http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.01/ Best regards, Martin
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi David,
On 2019-07-23 09:34, David Holmes wrote:
Hi Erik,
On 23/07/2019 5:10 pm, Erik Österlund wrote:
Hi Martin,
1) In the x86_64 assembly, you can combine the movl; testl; into a
single test instruction with one memory operand to the counter, and
one immediate zero.
2) If libjvm.so maps in far away, then the movl taking an
ExternalAddress, will actually scratch rscratch1, which is r10. That
will clobber the rcounter, and will at best cause all loads to take
the slow path. However, in the worst case, the subsequent
verification might say that the load was okay, even though it was not.
I was secretly hoping to never have to touch fast JNI getfield again,
because it is so shady, and the odd cases are very hard to test,
making it so easy to mess up. The ForceUnreachable JVM flag might be
useful in checking if a solution works also when rscratch1 gets
clobbered when referencing JVM symbols that are now "far away".
The subtle issue of referencing JVM symbols that can be far away,
suddenly clobbering r10, has bitten us many times. Perhaps it should
be made more explicit somehow. But that's a separate issue.
Too subtle for me. Is this issue written up anywhere? How do we know
what sequences are susceptible to this problem? How do we know when
the problem actually occurs? What is the fix?
1) Is this documented: Nope, buried deep in the code, where you least
expect to find it. A whole bunch of code asks if
Assembler::reachable(AddressLiteral adr), and depending on the answer
either perform a non-clobbering or a clobbering variation of the logical
instruction. This is precisely what happens with mov32:
void MacroAssembler::mov32(AddressLiteral dst, Register src) {
if (reachable(dst)) {
movl(as_Address(dst), src);
} else {
lea(rscratch1, dst); <-- this
is what will make things awkward
movl(Address(rscratch1, 0), src);
}
}
2) How do we know we are in trouble: Any macro assembler call that takes
an ExternalAddress parameter, might have to clobber r10. Rarely, when
the stars align, to make sure testing won't catch it.
3) When does it actually occur? When libjvm.so is mapped in further away
from the code cache than reachable in a signed integer, e.g. ~2 GB apart
in virtual address space. Tends to happen more often on windows it seems.
4) The fix that has been sadly applied all over the VM is to deal with
r10 being clobbered across such macro assembler instructions, either by
moving it to a place where it may safely be clobbered, or by stashing
away r10 across the macro assembler call and then restore it after. And
every now and then we forget this implicit side effect and things blow
up instead.
Also, I noticed that the counter that we are checking if it has
changed, is a 32 bit signed integer. They can actually wrap around,
which is undefined behaviour at best, and will make these tests fail
in the worst case. When we don't want counters to overflow, we use 64
bit integers.
Are you referring to the field-access counter? Changing that from a
32-bit to 64-bit value seems somewhat out-of-scope for the current
change, and may also have issues on 32-bit systems.
True. That might be okay the way it is.
Thanks,
/Erik
Thanks,
David
-
Thanks,
/Erik
On 2019-07-22 17:39, Doerr, Martin wrote:
Hi David and Erik,
I've tried to add the capability "can_generate_field_access_events"
during live phase and got "AddCapabilities failed with error 98"
which is "JVMTI_ERROR_NOT_AVAILABLE". So hotspot does not support
switching it on during live phase.
Hotspot initializes "can_generate_field_modification_events" during
"init_onload_solo_capabilities". As the name tells, it is
implemented as an "onload" capability.
So the VM works as expected with and without my change.
Can I add you as reviewers?
If yes, which parts did you review (x86, SPARC, shared code)?
Thanks and best regards,
Martin
-Original Message-
From: Doerr, Martin
Sent: Freitag, 19. Juli 2019 13:11
To: David Holmes ; hotspot-runtime-
d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik
Osterlund
Subject: RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI
field access
event requests at runtime
Hi David,
thanks for elaborating on the capability enablement.
With respect to "AddCapabilities", I've only found "Typically this
function is
used in the OnLoad function. Some virtual machines may allow a
limited set
of capabilities to be added in the live phase." in the spec [1].
I don't know which ones are supposed to be part of this "limited
set of
capabilities".
As you already explained, adding the capability for field access
events in the
live phase does obviously not work for hotspot.
The interpreter has the same issue.
Best regards,
Martin
[1]
https://docs.oracle.com/
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Erik, On 23/07/2019 5:10 pm, Erik Österlund wrote: Hi Martin, 1) In the x86_64 assembly, you can combine the movl; testl; into a single test instruction with one memory operand to the counter, and one immediate zero. 2) If libjvm.so maps in far away, then the movl taking an ExternalAddress, will actually scratch rscratch1, which is r10. That will clobber the rcounter, and will at best cause all loads to take the slow path. However, in the worst case, the subsequent verification might say that the load was okay, even though it was not. I was secretly hoping to never have to touch fast JNI getfield again, because it is so shady, and the odd cases are very hard to test, making it so easy to mess up. The ForceUnreachable JVM flag might be useful in checking if a solution works also when rscratch1 gets clobbered when referencing JVM symbols that are now "far away". The subtle issue of referencing JVM symbols that can be far away, suddenly clobbering r10, has bitten us many times. Perhaps it should be made more explicit somehow. But that's a separate issue. Too subtle for me. Is this issue written up anywhere? How do we know what sequences are susceptible to this problem? How do we know when the problem actually occurs? What is the fix? Also, I noticed that the counter that we are checking if it has changed, is a 32 bit signed integer. They can actually wrap around, which is undefined behaviour at best, and will make these tests fail in the worst case. When we don't want counters to overflow, we use 64 bit integers. Are you referring to the field-access counter? Changing that from a 32-bit to 64-bit value seems somewhat out-of-scope for the current change, and may also have issues on 32-bit systems. Thanks, David - Thanks, /Erik On 2019-07-22 17:39, Doerr, Martin wrote: Hi David and Erik, I've tried to add the capability "can_generate_field_access_events" during live phase and got "AddCapabilities failed with error 98" which is "JVMTI_ERROR_NOT_AVAILABLE". So hotspot does not support switching it on during live phase. Hotspot initializes "can_generate_field_modification_events" during "init_onload_solo_capabilities". As the name tells, it is implemented as an "onload" capability. So the VM works as expected with and without my change. Can I add you as reviewers? If yes, which parts did you review (x86, SPARC, shared code)? Thanks and best regards, Martin -Original Message- From: Doerr, Martin Sent: Freitag, 19. Juli 2019 13:11 To: David Holmes ; hotspot-runtime- d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik Osterlund Subject: RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime Hi David, thanks for elaborating on the capability enablement. With respect to "AddCapabilities", I've only found "Typically this function is used in the OnLoad function. Some virtual machines may allow a limited set of capabilities to be added in the live phase." in the spec [1]. I don't know which ones are supposed to be part of this "limited set of capabilities". As you already explained, adding the capability for field access events in the live phase does obviously not work for hotspot. The interpreter has the same issue. Best regards, Martin [1] https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#AddCapa bilities -Original Message- From: David Holmes Sent: Freitag, 19. Juli 2019 02:30 To: Doerr, Martin ; hotspot-runtime- d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik Osterlund Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime Hi Martin, On 18/07/2019 8:01 pm, Doerr, Martin wrote: Hi David and Erik, thank you for looking at my proposal. If you try to use fast field accessors when you have to post the field access event then how can you safely go off into a JVM TI event callback ?? We speculatively load the field and check afterwards if we can use this loaded value. It is safe to use it if there was no safepoint and no JVMTI event was requested. Otherwise, we simply discard the (possibly) loaded value and load it again in the slow path where we do all the synchronization and event posting. Thanks for clarifying for me. That is all fine then. The dynamics of this still concern me, but those concerns are also present in the existing code. Currently we don't use the quick accessors if JvmtiExport::can_post_field_access() is true during VM startup - this is a one-of initialization check that sets the use of fast accessors for the lifetime of the JVM. But that is set between the early-start and start VM events, before the live-phase. But AFAICS the capability for can_post_field_access can be set or cleared dynamically during the live phase, thus i
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
..small clarification on point #1, test against immediate -1 of course, not 0. /Erik > On 23 Jul 2019, at 09:10, Erik Österlund wrote: > > Hi Martin, > > 1) In the x86_64 assembly, you can combine the movl; testl; into a single > test instruction with one memory operand to the counter, and one immediate > zero. > > 2) If libjvm.so maps in far away, then the movl taking an ExternalAddress, > will actually scratch rscratch1, which is r10. That will clobber the > rcounter, and will at best cause all loads to take the slow path. However, in > the worst case, the subsequent verification might say that the load was okay, > even though it was not. > > I was secretly hoping to never have to touch fast JNI getfield again, because > it is so shady, and the odd cases are very hard to test, making it so easy to > mess up. The ForceUnreachable JVM flag might be useful in checking if a > solution works also when rscratch1 gets clobbered when referencing JVM > symbols that are now "far away". > > The subtle issue of referencing JVM symbols that can be far away, suddenly > clobbering r10, has bitten us many times. Perhaps it should be made more > explicit somehow. But that's a separate issue. > > Also, I noticed that the counter that we are checking if it has changed, is a > 32 bit signed integer. They can actually wrap around, which is undefined > behaviour at best, and will make these tests fail in the worst case. When we > don't want counters to overflow, we use 64 bit integers. > > Thanks, > /Erik > >> On 2019-07-22 17:39, Doerr, Martin wrote: >> Hi David and Erik, >> I've tried to add the capability "can_generate_field_access_events" during >> live phase and got "AddCapabilities failed with error 98" which is >> "JVMTI_ERROR_NOT_AVAILABLE". So hotspot does not support switching it on >> during live phase. >> Hotspot initializes "can_generate_field_modification_events" during >> "init_onload_solo_capabilities". As the name tells, it is implemented as an >> "onload" capability. >> So the VM works as expected with and without my change. >> Can I add you as reviewers? >> If yes, which parts did you review (x86, SPARC, shared code)? >> Thanks and best regards, >> Martin >>> -----Original Message- >>> From: Doerr, Martin >>> Sent: Freitag, 19. Juli 2019 13:11 >>> To: David Holmes ; hotspot-runtime- >>> d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik Osterlund >>> >>> Subject: RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access >>> event requests at runtime >>> >>> Hi David, >>> >>> thanks for elaborating on the capability enablement. >>> With respect to "AddCapabilities", I've only found "Typically this function >>> is >>> used in the OnLoad function. Some virtual machines may allow a limited set >>> of capabilities to be added in the live phase." in the spec [1]. >>> I don't know which ones are supposed to be part of this "limited set of >>> capabilities". >>> As you already explained, adding the capability for field access events in >>> the >>> live phase does obviously not work for hotspot. >>> The interpreter has the same issue. >>> >>> Best regards, >>> Martin >>> >>> >>> [1] >>> https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#AddCapa >>> bilities >>> >>> >>>> -Original Message- >>>> From: David Holmes >>>> Sent: Freitag, 19. Juli 2019 02:30 >>>> To: Doerr, Martin ; hotspot-runtime- >>>> d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik >>> Osterlund >>>> >>>> Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field >>> access >>>> event requests at runtime >>>> >>>> Hi Martin, >>>> >>>>> On 18/07/2019 8:01 pm, Doerr, Martin wrote: >>>>> Hi David and Erik, >>>>> >>>>> thank you for looking at my proposal. >>>>> >>>>>> If you try to use fast field accessors when you have to post the field >>>>>> access event then how can you safely go off into a JVM TI event callback >>>> ?? >>>>> >>>>> We speculatively load the field and check afterwards if we can use this >>>> loaded value. >>>&
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Martin, 1) In the x86_64 assembly, you can combine the movl; testl; into a single test instruction with one memory operand to the counter, and one immediate zero. 2) If libjvm.so maps in far away, then the movl taking an ExternalAddress, will actually scratch rscratch1, which is r10. That will clobber the rcounter, and will at best cause all loads to take the slow path. However, in the worst case, the subsequent verification might say that the load was okay, even though it was not. I was secretly hoping to never have to touch fast JNI getfield again, because it is so shady, and the odd cases are very hard to test, making it so easy to mess up. The ForceUnreachable JVM flag might be useful in checking if a solution works also when rscratch1 gets clobbered when referencing JVM symbols that are now "far away". The subtle issue of referencing JVM symbols that can be far away, suddenly clobbering r10, has bitten us many times. Perhaps it should be made more explicit somehow. But that's a separate issue. Also, I noticed that the counter that we are checking if it has changed, is a 32 bit signed integer. They can actually wrap around, which is undefined behaviour at best, and will make these tests fail in the worst case. When we don't want counters to overflow, we use 64 bit integers. Thanks, /Erik On 2019-07-22 17:39, Doerr, Martin wrote: Hi David and Erik, I've tried to add the capability "can_generate_field_access_events" during live phase and got "AddCapabilities failed with error 98" which is "JVMTI_ERROR_NOT_AVAILABLE". So hotspot does not support switching it on during live phase. Hotspot initializes "can_generate_field_modification_events" during "init_onload_solo_capabilities". As the name tells, it is implemented as an "onload" capability. So the VM works as expected with and without my change. Can I add you as reviewers? If yes, which parts did you review (x86, SPARC, shared code)? Thanks and best regards, Martin -Original Message- From: Doerr, Martin Sent: Freitag, 19. Juli 2019 13:11 To: David Holmes ; hotspot-runtime- d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik Osterlund Subject: RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime Hi David, thanks for elaborating on the capability enablement. With respect to "AddCapabilities", I've only found "Typically this function is used in the OnLoad function. Some virtual machines may allow a limited set of capabilities to be added in the live phase." in the spec [1]. I don't know which ones are supposed to be part of this "limited set of capabilities". As you already explained, adding the capability for field access events in the live phase does obviously not work for hotspot. The interpreter has the same issue. Best regards, Martin [1] https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#AddCapa bilities -Original Message- From: David Holmes Sent: Freitag, 19. Juli 2019 02:30 To: Doerr, Martin ; hotspot-runtime- d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik Osterlund Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime Hi Martin, On 18/07/2019 8:01 pm, Doerr, Martin wrote: Hi David and Erik, thank you for looking at my proposal. If you try to use fast field accessors when you have to post the field access event then how can you safely go off into a JVM TI event callback ?? We speculatively load the field and check afterwards if we can use this loaded value. It is safe to use it if there was no safepoint and no JVMTI event was requested. Otherwise, we simply discard the (possibly) loaded value and load it again in the slow path where we do all the synchronization and event posting. Thanks for clarifying for me. That is all fine then. The dynamics of this still concern me, but those concerns are also present in the existing code. Currently we don't use the quick accessors if JvmtiExport::can_post_field_access() is true during VM startup - this is a one-of initialization check that sets the use of fast accessors for the lifetime of the JVM. But that is set between the early-start and start VM events, before the live-phase. But AFAICS the capability for can_post_field_access can be set or cleared dynamically during the live phase, thus invalidating the original decision on whether to use fast accessors or not. With your changes the state of can_post_field_access is still captured during VM initialization so again the decision to check for a field access watch is hard-wired for the lifetime of the VM. But once installed that check allows for use of the fast-path if no actual watches are set - which is the whole point of this enhancement. So the issue with both old and new code is that if the capability is
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Martin, On 23/07/2019 1:39 am, Doerr, Martin wrote: Hi David and Erik, I've tried to add the capability "can_generate_field_access_events" during live phase and got "AddCapabilities failed with error 98" which is "JVMTI_ERROR_NOT_AVAILABLE". So hotspot does not support switching it on during live phase. Hotspot initializes "can_generate_field_modification_events" during "init_onload_solo_capabilities". As the name tells, it is implemented as an "onload" capability. So the VM works as expected with and without my change. Okay - thanks for verifying that. Can I add you as reviewers? If yes, which parts did you review (x86, SPARC, shared code)? I reviewed x86, sparc and shared code. Thanks, David Thanks and best regards, Martin -Original Message- From: Doerr, Martin Sent: Freitag, 19. Juli 2019 13:11 To: David Holmes ; hotspot-runtime- d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik Osterlund Subject: RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime Hi David, thanks for elaborating on the capability enablement. With respect to "AddCapabilities", I've only found "Typically this function is used in the OnLoad function. Some virtual machines may allow a limited set of capabilities to be added in the live phase." in the spec [1]. I don't know which ones are supposed to be part of this "limited set of capabilities". As you already explained, adding the capability for field access events in the live phase does obviously not work for hotspot. The interpreter has the same issue. Best regards, Martin [1] https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#AddCapa bilities -Original Message- From: David Holmes Sent: Freitag, 19. Juli 2019 02:30 To: Doerr, Martin ; hotspot-runtime- d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik Osterlund Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime Hi Martin, On 18/07/2019 8:01 pm, Doerr, Martin wrote: Hi David and Erik, thank you for looking at my proposal. If you try to use fast field accessors when you have to post the field access event then how can you safely go off into a JVM TI event callback ?? We speculatively load the field and check afterwards if we can use this loaded value. It is safe to use it if there was no safepoint and no JVMTI event was requested. Otherwise, we simply discard the (possibly) loaded value and load it again in the slow path where we do all the synchronization and event posting. Thanks for clarifying for me. That is all fine then. The dynamics of this still concern me, but those concerns are also present in the existing code. Currently we don't use the quick accessors if JvmtiExport::can_post_field_access() is true during VM startup - this is a one-of initialization check that sets the use of fast accessors for the lifetime of the JVM. But that is set between the early-start and start VM events, before the live-phase. But AFAICS the capability for can_post_field_access can be set or cleared dynamically during the live phase, thus invalidating the original decision on whether to use fast accessors or not. With your changes the state of can_post_field_access is still captured during VM initialization so again the decision to check for a field access watch is hard-wired for the lifetime of the VM. But once installed that check allows for use of the fast-path if no actual watches are set - which is the whole point of this enhancement. So the issue with both old and new code is that if the capability is not present at VM startup the VM will be configured to always use the fast path, even if the capability (and field access watches) are added later. Thanks, David @Erik: Thanks for your proposal to change the function pointers. I'll look into that. Best regards, Martin -Original Message- From: David Holmes Sent: Donnerstag, 18. Juli 2019 06:39 To: Doerr, Martin ; hotspot-runtime- d...@openjdk.java.net; serviceability-dev@openjdk.java.net Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime Hi Martin, I need to think about this some more. A critical property of the fast field accessors are that they are trivial and completely safe. They are complicated by the need to check if a GC may have happened while we directly read the field. If you try to use fast field accessors when you have to post the field access event then how can you safely go off into a JVM TI event callback ?? Thanks, David On 16/07/2019 11:31 pm, Doerr, Martin wrote: Hi, the current implementation of FastJNIAccessors ignores the flag - XX:+UseFastJNIAccessors when the JVMTI capability "can_post_field_access" is enabled. This is an unnecessary restriction which makes
RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi David and Erik, I've tried to add the capability "can_generate_field_access_events" during live phase and got "AddCapabilities failed with error 98" which is "JVMTI_ERROR_NOT_AVAILABLE". So hotspot does not support switching it on during live phase. Hotspot initializes "can_generate_field_modification_events" during "init_onload_solo_capabilities". As the name tells, it is implemented as an "onload" capability. So the VM works as expected with and without my change. Can I add you as reviewers? If yes, which parts did you review (x86, SPARC, shared code)? Thanks and best regards, Martin > -Original Message- > From: Doerr, Martin > Sent: Freitag, 19. Juli 2019 13:11 > To: David Holmes ; hotspot-runtime- > d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik Osterlund > > Subject: RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access > event requests at runtime > > Hi David, > > thanks for elaborating on the capability enablement. > With respect to "AddCapabilities", I've only found "Typically this function is > used in the OnLoad function. Some virtual machines may allow a limited set > of capabilities to be added in the live phase." in the spec [1]. > I don't know which ones are supposed to be part of this "limited set of > capabilities". > As you already explained, adding the capability for field access events in the > live phase does obviously not work for hotspot. > The interpreter has the same issue. > > Best regards, > Martin > > > [1] > https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#AddCapa > bilities > > > > -Original Message- > > From: David Holmes > > Sent: Freitag, 19. Juli 2019 02:30 > > To: Doerr, Martin ; hotspot-runtime- > > d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik > Osterlund > > > > Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field > access > > event requests at runtime > > > > Hi Martin, > > > > On 18/07/2019 8:01 pm, Doerr, Martin wrote: > > > Hi David and Erik, > > > > > > thank you for looking at my proposal. > > > > > >> If you try to use fast field accessors when you have to post the field > > >> access event then how can you safely go off into a JVM TI event callback > > ?? > > > > > > We speculatively load the field and check afterwards if we can use this > > loaded value. > > > It is safe to use it if there was no safepoint and no JVMTI event was > > requested. > > > Otherwise, we simply discard the (possibly) loaded value and load it again > > in the slow path where we do all the synchronization and event posting. > > > > Thanks for clarifying for me. That is all fine then. > > > > The dynamics of this still concern me, but those concerns are also > > present in the existing code. Currently we don't use the quick accessors > > if JvmtiExport::can_post_field_access() is true during VM startup - this > > is a one-of initialization check that sets the use of fast accessors for > > the lifetime of the JVM. But that is set between the early-start and > > start VM events, before the live-phase. But AFAICS the capability for > > can_post_field_access can be set or cleared dynamically during the live > > phase, thus invalidating the original decision on whether to use fast > > accessors or not. With your changes the state of can_post_field_access > > is still captured during VM initialization so again the decision to > > check for a field access watch is hard-wired for the lifetime of the VM. > > But once installed that check allows for use of the fast-path if no > > actual watches are set - which is the whole point of this enhancement. > > So the issue with both old and new code is that if the capability is not > > present at VM startup the VM will be configured to always use the fast > > path, even if the capability (and field access watches) are added later. > > > > Thanks, > > David > > > > > @Erik: > > > Thanks for your proposal to change the function pointers. I'll look into > that. > > > > > > Best regards, > > > Martin > > > > > > > > >> -Original Message- > > >> From: David Holmes > > >> Sent: Donnerstag, 18. Juli 2019 06:39 > > >> To: Doerr, Martin ; hotspot-runtime- > > >> d...@openjdk.java.net; serviceability-dev@openjdk.java.net > > >> Subject: Re: RFR(M): 8227680: FastJ
RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi David, thanks for elaborating on the capability enablement. With respect to "AddCapabilities", I've only found "Typically this function is used in the OnLoad function. Some virtual machines may allow a limited set of capabilities to be added in the live phase." in the spec [1]. I don't know which ones are supposed to be part of this "limited set of capabilities". As you already explained, adding the capability for field access events in the live phase does obviously not work for hotspot. The interpreter has the same issue. Best regards, Martin [1] https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#AddCapabilities > -Original Message- > From: David Holmes > Sent: Freitag, 19. Juli 2019 02:30 > To: Doerr, Martin ; hotspot-runtime- > d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik Osterlund > > Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access > event requests at runtime > > Hi Martin, > > On 18/07/2019 8:01 pm, Doerr, Martin wrote: > > Hi David and Erik, > > > > thank you for looking at my proposal. > > > >> If you try to use fast field accessors when you have to post the field > >> access event then how can you safely go off into a JVM TI event callback > ?? > > > > We speculatively load the field and check afterwards if we can use this > loaded value. > > It is safe to use it if there was no safepoint and no JVMTI event was > requested. > > Otherwise, we simply discard the (possibly) loaded value and load it again > in the slow path where we do all the synchronization and event posting. > > Thanks for clarifying for me. That is all fine then. > > The dynamics of this still concern me, but those concerns are also > present in the existing code. Currently we don't use the quick accessors > if JvmtiExport::can_post_field_access() is true during VM startup - this > is a one-of initialization check that sets the use of fast accessors for > the lifetime of the JVM. But that is set between the early-start and > start VM events, before the live-phase. But AFAICS the capability for > can_post_field_access can be set or cleared dynamically during the live > phase, thus invalidating the original decision on whether to use fast > accessors or not. With your changes the state of can_post_field_access > is still captured during VM initialization so again the decision to > check for a field access watch is hard-wired for the lifetime of the VM. > But once installed that check allows for use of the fast-path if no > actual watches are set - which is the whole point of this enhancement. > So the issue with both old and new code is that if the capability is not > present at VM startup the VM will be configured to always use the fast > path, even if the capability (and field access watches) are added later. > > Thanks, > David > > > @Erik: > > Thanks for your proposal to change the function pointers. I'll look into > > that. > > > > Best regards, > > Martin > > > > > >> -Original Message- > >> From: David Holmes > >> Sent: Donnerstag, 18. Juli 2019 06:39 > >> To: Doerr, Martin ; hotspot-runtime- > >> d...@openjdk.java.net; serviceability-dev@openjdk.java.net > >> Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field > access > >> event requests at runtime > >> > >> Hi Martin, > >> > >> I need to think about this some more. A critical property of the fast > >> field accessors are that they are trivial and completely safe. They are > >> complicated by the need to check if a GC may have happened while we > >> directly read the field. > >> > >> If you try to use fast field accessors when you have to post the field > >> access event then how can you safely go off into a JVM TI event callback > ?? > >> > >> Thanks, > >> David > >> > >> On 16/07/2019 11:31 pm, Doerr, Martin wrote: > >>> Hi, > >>> > >>> the current implementation of FastJNIAccessors ignores the flag - > >> XX:+UseFastJNIAccessors when the JVMTI capability > >> "can_post_field_access" is enabled. > >>> This is an unnecessary restriction which makes field accesses > >> (GetField) from native code slower when a JVMTI agent is > attached > >> which enables this capability. > >>> A better implementation would check at runtime if an agent actually > wants > >> to receive field access events. > >>> > >>> Note that the bytecode interpreter alr
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Martin, On 18/07/2019 8:01 pm, Doerr, Martin wrote: Hi David and Erik, thank you for looking at my proposal. If you try to use fast field accessors when you have to post the field access event then how can you safely go off into a JVM TI event callback ?? We speculatively load the field and check afterwards if we can use this loaded value. It is safe to use it if there was no safepoint and no JVMTI event was requested. Otherwise, we simply discard the (possibly) loaded value and load it again in the slow path where we do all the synchronization and event posting. Thanks for clarifying for me. That is all fine then. The dynamics of this still concern me, but those concerns are also present in the existing code. Currently we don't use the quick accessors if JvmtiExport::can_post_field_access() is true during VM startup - this is a one-of initialization check that sets the use of fast accessors for the lifetime of the JVM. But that is set between the early-start and start VM events, before the live-phase. But AFAICS the capability for can_post_field_access can be set or cleared dynamically during the live phase, thus invalidating the original decision on whether to use fast accessors or not. With your changes the state of can_post_field_access is still captured during VM initialization so again the decision to check for a field access watch is hard-wired for the lifetime of the VM. But once installed that check allows for use of the fast-path if no actual watches are set - which is the whole point of this enhancement. So the issue with both old and new code is that if the capability is not present at VM startup the VM will be configured to always use the fast path, even if the capability (and field access watches) are added later. Thanks, David @Erik: Thanks for your proposal to change the function pointers. I'll look into that. Best regards, Martin -Original Message- From: David Holmes Sent: Donnerstag, 18. Juli 2019 06:39 To: Doerr, Martin ; hotspot-runtime- d...@openjdk.java.net; serviceability-dev@openjdk.java.net Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime Hi Martin, I need to think about this some more. A critical property of the fast field accessors are that they are trivial and completely safe. They are complicated by the need to check if a GC may have happened while we directly read the field. If you try to use fast field accessors when you have to post the field access event then how can you safely go off into a JVM TI event callback ?? Thanks, David On 16/07/2019 11:31 pm, Doerr, Martin wrote: Hi, the current implementation of FastJNIAccessors ignores the flag - XX:+UseFastJNIAccessors when the JVMTI capability "can_post_field_access" is enabled. This is an unnecessary restriction which makes field accesses (GetField) from native code slower when a JVMTI agent is attached which enables this capability. A better implementation would check at runtime if an agent actually wants to receive field access events. Note that the bytecode interpreter already uses this better implementation by checking if field access watch events were requested (JvmtiExport::_field_access_count != 0). I have implemented such a runtime check on all platforms which currently support FastJNIAccessors. My new jtreg test runtime/jni/FastGetField/FastGetField.java contains a micro benchmark: test- support/jtreg_test_hotspot_jtreg_runtime_jni_FastGetField/runtime/jni/Fa stGetField/FastGetField.jtr shows the duration of 1 iterations with and without UseFastJNIAccessors (JVMTI agent gets attached in both runs). My Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz needed 4.7ms with FastJNIAccessors and 11.2ms without it. Webrev: http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.00/ We have run the test on 64 bit x86 platforms, SPARC and aarch64. (FastJNIAccessors are not yet available on PPC64 and s390. I'll contribute them later.) My webrev contains 32 bit implementations for x86 and arm, but completely untested. It'd be great if somebody could volunteer to review and test these platforms. Please review. Best regards, Martin
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Martin, Okay, looks good in that case. Thanks, /Erik > On 18 Jul 2019, at 14:51, Doerr, Martin wrote: > > Hi Erik, > > I like the idea, but it seems to be difficult. > > JNI function table can get copied and redirected at runtime (e.g. > SetJNIFunctionTable). > We'd have to synchronize with that to avoid messing it up. > > Also, I think the function pointers should better be made volatile if we > change them concurrently. > > I have to think more about all of that, but I guess this approach will be > more complicated than my initial proposal. > > Best regards, > Martin > > >> -Original Message- >> From: Erik Osterlund >> Sent: Donnerstag, 18. Juli 2019 08:43 >> To: Doerr, Martin >> Cc: hotspot-runtime-...@openjdk.java.net; serviceability- >> d...@openjdk.java.net >> Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access >> event requests at runtime >> >> Hi Martin, >> >> Since the JNI calls go through function pointers in the JNI env that go >> either >> to the fast or slow version, could one option be to go through the JNI envs >> and change the function pointers to the slow one when this JVMTI feature is >> enabled? >> >> Advantages: >> 1) No need to change the platform specific code that seems to surprisingly >> work right now. >> 2) No need for the fast path to check that condition. >> >> Just an idea. >> >> Thanks, >> /Erik >> >> >>> On 16 Jul 2019, at 15:31, Doerr, Martin wrote: >>> >>> Hi, >>> >>> the current implementation of FastJNIAccessors ignores the flag - >> XX:+UseFastJNIAccessors when the JVMTI capability >> "can_post_field_access" is enabled. >>> This is an unnecessary restriction which makes field accesses >> (GetField) from native code slower when a JVMTI agent is attached >> which enables this capability. >>> A better implementation would check at runtime if an agent actually wants >> to receive field access events. >>> >>> Note that the bytecode interpreter already uses this better >> implementation by checking if field access watch events were requested >> (JvmtiExport::_field_access_count != 0). >>> >>> I have implemented such a runtime check on all platforms which currently >> support FastJNIAccessors. >>> >>> My new jtreg test runtime/jni/FastGetField/FastGetField.java contains a >> micro benchmark: >>> test- >> support/jtreg_test_hotspot_jtreg_runtime_jni_FastGetField/runtime/jni/Fa >> stGetField/FastGetField.jtr >>> shows the duration of 1 iterations with and without >> UseFastJNIAccessors (JVMTI agent gets attached in both runs). >>> My Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz needed 4.7ms with >> FastJNIAccessors and 11.2ms without it. >>> >>> Webrev: >>> >> http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.00/ >>> >>> We have run the test on 64 bit x86 platforms, SPARC and aarch64. >>> (FastJNIAccessors are not yet available on PPC64 and s390. I'll contribute >> them later.) >>> My webrev contains 32 bit implementations for x86 and arm, but >> completely untested. It'd be great if somebody could volunteer to review >> and test these platforms. >>> >>> Please review. >>> >>> Best regards, >>> Martin >>> >
RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Erik, I like the idea, but it seems to be difficult. JNI function table can get copied and redirected at runtime (e.g. SetJNIFunctionTable). We'd have to synchronize with that to avoid messing it up. Also, I think the function pointers should better be made volatile if we change them concurrently. I have to think more about all of that, but I guess this approach will be more complicated than my initial proposal. Best regards, Martin > -Original Message- > From: Erik Osterlund > Sent: Donnerstag, 18. Juli 2019 08:43 > To: Doerr, Martin > Cc: hotspot-runtime-...@openjdk.java.net; serviceability- > d...@openjdk.java.net > Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access > event requests at runtime > > Hi Martin, > > Since the JNI calls go through function pointers in the JNI env that go either > to the fast or slow version, could one option be to go through the JNI envs > and change the function pointers to the slow one when this JVMTI feature is > enabled? > > Advantages: > 1) No need to change the platform specific code that seems to surprisingly > work right now. > 2) No need for the fast path to check that condition. > > Just an idea. > > Thanks, > /Erik > > > > On 16 Jul 2019, at 15:31, Doerr, Martin wrote: > > > > Hi, > > > > the current implementation of FastJNIAccessors ignores the flag - > XX:+UseFastJNIAccessors when the JVMTI capability > "can_post_field_access" is enabled. > > This is an unnecessary restriction which makes field accesses > (GetField) from native code slower when a JVMTI agent is attached > which enables this capability. > > A better implementation would check at runtime if an agent actually wants > to receive field access events. > > > > Note that the bytecode interpreter already uses this better > implementation by checking if field access watch events were requested > (JvmtiExport::_field_access_count != 0). > > > > I have implemented such a runtime check on all platforms which currently > support FastJNIAccessors. > > > > My new jtreg test runtime/jni/FastGetField/FastGetField.java contains a > micro benchmark: > > test- > support/jtreg_test_hotspot_jtreg_runtime_jni_FastGetField/runtime/jni/Fa > stGetField/FastGetField.jtr > > shows the duration of 1 iterations with and without > UseFastJNIAccessors (JVMTI agent gets attached in both runs). > > My Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz needed 4.7ms with > FastJNIAccessors and 11.2ms without it. > > > > Webrev: > > > http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.00/ > > > > We have run the test on 64 bit x86 platforms, SPARC and aarch64. > > (FastJNIAccessors are not yet available on PPC64 and s390. I'll contribute > them later.) > > My webrev contains 32 bit implementations for x86 and arm, but > completely untested. It'd be great if somebody could volunteer to review > and test these platforms. > > > > Please review. > > > > Best regards, > > Martin > >
RE: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi David and Erik, thank you for looking at my proposal. > If you try to use fast field accessors when you have to post the field > access event then how can you safely go off into a JVM TI event callback ?? We speculatively load the field and check afterwards if we can use this loaded value. It is safe to use it if there was no safepoint and no JVMTI event was requested. Otherwise, we simply discard the (possibly) loaded value and load it again in the slow path where we do all the synchronization and event posting. @Erik: Thanks for your proposal to change the function pointers. I'll look into that. Best regards, Martin > -Original Message- > From: David Holmes > Sent: Donnerstag, 18. Juli 2019 06:39 > To: Doerr, Martin ; hotspot-runtime- > d...@openjdk.java.net; serviceability-dev@openjdk.java.net > Subject: Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access > event requests at runtime > > Hi Martin, > > I need to think about this some more. A critical property of the fast > field accessors are that they are trivial and completely safe. They are > complicated by the need to check if a GC may have happened while we > directly read the field. > > If you try to use fast field accessors when you have to post the field > access event then how can you safely go off into a JVM TI event callback ?? > > Thanks, > David > > On 16/07/2019 11:31 pm, Doerr, Martin wrote: > > Hi, > > > > the current implementation of FastJNIAccessors ignores the flag - > XX:+UseFastJNIAccessors when the JVMTI capability > "can_post_field_access" is enabled. > > This is an unnecessary restriction which makes field accesses > (GetField) from native code slower when a JVMTI agent is attached > which enables this capability. > > A better implementation would check at runtime if an agent actually wants > to receive field access events. > > > > Note that the bytecode interpreter already uses this better > implementation by checking if field access watch events were requested > (JvmtiExport::_field_access_count != 0). > > > > I have implemented such a runtime check on all platforms which currently > support FastJNIAccessors. > > > > My new jtreg test runtime/jni/FastGetField/FastGetField.java contains a > micro benchmark: > > test- > support/jtreg_test_hotspot_jtreg_runtime_jni_FastGetField/runtime/jni/Fa > stGetField/FastGetField.jtr > > shows the duration of 1 iterations with and without > UseFastJNIAccessors (JVMTI agent gets attached in both runs). > > My Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz needed 4.7ms with > FastJNIAccessors and 11.2ms without it. > > > > Webrev: > > > http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.00/ > > > > We have run the test on 64 bit x86 platforms, SPARC and aarch64. > > (FastJNIAccessors are not yet available on PPC64 and s390. I'll contribute > them later.) > > My webrev contains 32 bit implementations for x86 and arm, but > completely untested. It'd be great if somebody could volunteer to review > and test these platforms. > > > > Please review. > > > > Best regards, > > Martin > >
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Martin, Since the JNI calls go through function pointers in the JNI env that go either to the fast or slow version, could one option be to go through the JNI envs and change the function pointers to the slow one when this JVMTI feature is enabled? Advantages: 1) No need to change the platform specific code that seems to surprisingly work right now. 2) No need for the fast path to check that condition. Just an idea. Thanks, /Erik > On 16 Jul 2019, at 15:31, Doerr, Martin wrote: > > Hi, > > the current implementation of FastJNIAccessors ignores the flag > -XX:+UseFastJNIAccessors when the JVMTI capability "can_post_field_access" is > enabled. > This is an unnecessary restriction which makes field accesses > (GetField) from native code slower when a JVMTI agent is attached which > enables this capability. > A better implementation would check at runtime if an agent actually wants to > receive field access events. > > Note that the bytecode interpreter already uses this better implementation by > checking if field access watch events were requested > (JvmtiExport::_field_access_count != 0). > > I have implemented such a runtime check on all platforms which currently > support FastJNIAccessors. > > My new jtreg test runtime/jni/FastGetField/FastGetField.java contains a micro > benchmark: > test-support/jtreg_test_hotspot_jtreg_runtime_jni_FastGetField/runtime/jni/FastGetField/FastGetField.jtr > shows the duration of 1 iterations with and without UseFastJNIAccessors > (JVMTI agent gets attached in both runs). > My Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz needed 4.7ms with > FastJNIAccessors and 11.2ms without it. > > Webrev: > http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.00/ > > We have run the test on 64 bit x86 platforms, SPARC and aarch64. > (FastJNIAccessors are not yet available on PPC64 and s390. I'll contribute > them later.) > My webrev contains 32 bit implementations for x86 and arm, but completely > untested. It'd be great if somebody could volunteer to review and test these > platforms. > > Please review. > > Best regards, > Martin >
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Martin, I need to think about this some more. A critical property of the fast field accessors are that they are trivial and completely safe. They are complicated by the need to check if a GC may have happened while we directly read the field. If you try to use fast field accessors when you have to post the field access event then how can you safely go off into a JVM TI event callback ?? Thanks, David On 16/07/2019 11:31 pm, Doerr, Martin wrote: Hi, the current implementation of FastJNIAccessors ignores the flag -XX:+UseFastJNIAccessors when the JVMTI capability "can_post_field_access" is enabled. This is an unnecessary restriction which makes field accesses (GetField) from native code slower when a JVMTI agent is attached which enables this capability. A better implementation would check at runtime if an agent actually wants to receive field access events. Note that the bytecode interpreter already uses this better implementation by checking if field access watch events were requested (JvmtiExport::_field_access_count != 0). I have implemented such a runtime check on all platforms which currently support FastJNIAccessors. My new jtreg test runtime/jni/FastGetField/FastGetField.java contains a micro benchmark: test-support/jtreg_test_hotspot_jtreg_runtime_jni_FastGetField/runtime/jni/FastGetField/FastGetField.jtr shows the duration of 1 iterations with and without UseFastJNIAccessors (JVMTI agent gets attached in both runs). My Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz needed 4.7ms with FastJNIAccessors and 11.2ms without it. Webrev: http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.00/ We have run the test on 64 bit x86 platforms, SPARC and aarch64. (FastJNIAccessors are not yet available on PPC64 and s390. I'll contribute them later.) My webrev contains 32 bit implementations for x86 and arm, but completely untested. It'd be great if somebody could volunteer to review and test these platforms. Please review. Best regards, Martin
RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi, the current implementation of FastJNIAccessors ignores the flag -XX:+UseFastJNIAccessors when the JVMTI capability "can_post_field_access" is enabled. This is an unnecessary restriction which makes field accesses (GetField) from native code slower when a JVMTI agent is attached which enables this capability. A better implementation would check at runtime if an agent actually wants to receive field access events. Note that the bytecode interpreter already uses this better implementation by checking if field access watch events were requested (JvmtiExport::_field_access_count != 0). I have implemented such a runtime check on all platforms which currently support FastJNIAccessors. My new jtreg test runtime/jni/FastGetField/FastGetField.java contains a micro benchmark: test-support/jtreg_test_hotspot_jtreg_runtime_jni_FastGetField/runtime/jni/FastGetField/FastGetField.jtr shows the duration of 1 iterations with and without UseFastJNIAccessors (JVMTI agent gets attached in both runs). My Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz needed 4.7ms with FastJNIAccessors and 11.2ms without it. Webrev: http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.00/ We have run the test on 64 bit x86 platforms, SPARC and aarch64. (FastJNIAccessors are not yet available on PPC64 and s390. I'll contribute them later.) My webrev contains 32 bit implementations for x86 and arm, but completely untested. It'd be great if somebody could volunteer to review and test these platforms. Please review. Best regards, Martin
