RFR 8242263: Diagnose synchronization on primitive wrappers

[Patricio Chilano]

Hi Martin,

On 8/7/20 4:33 PM, Doerr, Martin wrote:
>
> Hi Patricio,
>
> thanks for incorporating my fixes.
>
> I don’t know if I can find time for measurements next week. I have a 
> lot of other things to do.
>
> Feel free to push it after you got enough reviews.
>
Ok, I'll do. Thanks for reviewing this!


Patricio
>
> Best regards,
>
> Martin
>
> *From:*Patricio Chilano <patricio.chilano.mateo at oracle.com>
> *Sent:* Freitag, 7. August 2020 17:29
> *To:* Doerr, Martin <martin.doerr at sap.com>; 
> daniel.daugherty at oracle.com; David Holmes <david.holmes at oracle.com>; 
> hotspot-runtime-dev at openjdk.java.net; hotspot-jfr-dev at openjdk.java.net
> *Subject:* Re: RFR 8242263: Diagnose synchronization on primitive wrappers
>
> Hi Martin,
>
> On 8/7/20 12:58 AM, Doerr, Martin wrote:
>
>     Hi Patricio,
>
>     thanks for the update.
>
>     Sorry for causing trouble related to the tbnz instructions on aarch64.
>
>     My concern was basically to use the correct number of Bytes for the load instructions.
>
>     You have fixed that in webrev v3. Thanks.
>
>     Unfortunately, I did the same mistake regarding the flag requirements on PPC64 and s390. And we have a wrong label on PPC64 (otherwise branch destination can be too far).
>
>     Test duration is still terrible with the slow debug build. I still got timeouts. (Maybe I should better use fast debug builds.)
>
> I measured the time in my Mac (fastdebug build). The FatalTest takes ~ 
> less than a second to run, and we have 7*8=56 of those. The LogTest 
> takes between 1-4 seconds depending on the flag, and we have 7 of 
> those. Overall it takes 2-3 minutes to run. Maybe you can measure 
> those times too to see where its taking so long?
>
>
>     Fixes see below.
>
> Great, added the new fixes. Here is v4:
>
> Full: http://cr.openjdk.java.net/~pchilanomate/8242263/v4/webrev/
> Inc: http://cr.openjdk.java.net/~pchilanomate/8242263/v4/inc/webrev/ 
> <http://cr.openjdk.java.net/~pchilanomate/8242263/v4/webrev/>
>
>
>     Sometimes simple changes can require quite some time ...
>
> Yep, no problem  : )
>
> Thanks!
>
>
> Patricio
>
>     Best regards,
>
>     Martin
>
>     diff -r f629202b3862 src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp
>
>     --- a/src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp     Thu Aug 06 22:03:39 2020 +0200
>
>     +++ b/src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp     Thu Aug 06 23:26:02 2020 +0200
>
>     @@ -109,7 +109,7 @@
>
>           load_klass(Rscratch, Roop);
>
>           lwz(Rscratch, in_bytes(Klass::access_flags_offset()), Rscratch);
>
>           testbitdi(CCR0, R0, Rscratch, exact_log2(JVM_ACC_IS_BOX_CLASS));
>
>     -    bne(CCR0, slow_case);
>
>     +    bne(CCR0, slow_int);
>
>         }
>
>         if (UseBiasedLocking) {
>
>     diff -r f629202b3862 src/hotspot/cpu/ppc/macroAssembler_ppc.cpp
>
>     --- a/src/hotspot/cpu/ppc/macroAssembler_ppc.cpp        Thu Aug 06 22:03:39 2020 +0200
>
>     +++ b/src/hotspot/cpu/ppc/macroAssembler_ppc.cpp        Thu Aug 06 23:26:02 2020 +0200
>
>     @@ -2839,8 +2839,8 @@
>
>         if (DiagnoseSyncOnPrimitiveWrappers != 0) {
>
>           load_klass(temp, oop);
>
>           lwz(temp, in_bytes(Klass::access_flags_offset()), temp);
>
>     -    testbitdi(CCR0, R0, temp, exact_log2(JVM_ACC_IS_BOX_CLASS));
>
>     -    bne(CCR0, cont);
>
>     +    testbitdi(flag, R0, temp, exact_log2(JVM_ACC_IS_BOX_CLASS));
>
>     +    bne(flag, cont);
>
>         }
>
>         if (try_bias) {
>
>     diff -r f629202b3862 src/hotspot/cpu/s390/macroAssembler_s390.cpp
>
>     --- a/src/hotspot/cpu/s390/macroAssembler_s390.cpp      Thu Aug 06 22:03:39 2020 +0200
>
>     +++ b/src/hotspot/cpu/s390/macroAssembler_s390.cpp      Thu Aug 06 23:26:02 2020 +0200
>
>     @@ -3360,8 +3360,10 @@
>
>         if (DiagnoseSyncOnPrimitiveWrappers != 0) {
>
>           load_klass(Z_R1_scratch, oop);
>
>     -    testbit(Address(Z_R1_scratch, Klass::access_flags_offset()), exact_log2(JVM_ACC_IS_BOX_CLASS));
>
>     -    z_btrue(done);
>
>     +    z_l(Z_R1_scratch, Address(Z_R1_scratch, Klass::access_flags_offset()));
>
>     +    assert((JVM_ACC_IS_BOX_CLASS & 0xFFFF) == 0, "or change following instruction");
>
>     +    z_nilh(Z_R1_scratch, JVM_ACC_IS_BOX_CLASS >> 16);
>
>     +    z_brne(done);
>
>         }
>
>         if (try_bias) {
>
>     diff -r f629202b3862 test/hotspot/jtreg/runtime/Monitor/SyncOnPrimitiveWrapperTest.java
>
>     --- a/test/hotspot/jtreg/runtime/Monitor/SyncOnPrimitiveWrapperTest.java        Thu Aug 06 22:03:39 2020 +0200
>
>     +++ b/test/hotspot/jtreg/runtime/Monitor/SyncOnPrimitiveWrapperTest.java        Thu Aug 06 23:26:02 2020 +0200
>
>     @@ -31,7 +31,7 @@
>
>        * @bug 8242263
>
>        * @summary Exercise DiagnoseSyncOnPrimitiveWrappers diagnostic flag
>
>        * @library /test/lib
>
>     - * @run driver SyncOnPrimitiveWrapperTest
>
>     + * @run driver/timeout=180000 SyncOnPrimitiveWrapperTest
>
>        */
>
>       public class SyncOnPrimitiveWrapperTest {
>