..small clarification on point #1, test against immediate -1 of course, not 0.
/Erik > On 23 Jul 2019, at 09:10, Erik Österlund <erik.osterl...@oracle.com> 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 <david.hol...@oracle.com>; hotspot-runtime- >>> d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik Osterlund >>> <erik.osterl...@oracle.com> >>> 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 <david.hol...@oracle.com> >>>> Sent: Freitag, 19. Juli 2019 02:30 >>>> To: Doerr, Martin <martin.do...@sap.com>; hotspot-runtime- >>>> d...@openjdk.java.net; serviceability-dev@openjdk.java.net; Erik >>> Osterlund >>>> <erik.osterl...@oracle.com> >>>> 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 <david.hol...@oracle.com> >>>>>> Sent: Donnerstag, 18. Juli 2019 06:39 >>>>>> To: Doerr, Martin <martin.do...@sap.com>; 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 >>>>>> (Get<Type>Field) 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 10000 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 >>>>>>>
