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 <boris.ulasev...@bell-sw.com> > Sent: Freitag, 26. Juli 2019 18:18 > To: Doerr, Martin <martin.do...@sap.com> > 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 <boris.ulasev...@bell-sw.com> > >> Sent: Freitag, 26. Juli 2019 12:50 > >> To: Doerr, Martin <martin.do...@sap.com> > >> 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 > >> (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 > >>>
