RFR 8242263: Diagnose synchronization on primitive wrappers
Patricio Chilano
patricio.chilano.mateo at oracle.com
Wed Aug 5 13:01:34 UTC 2020
Hi Martin,
On 8/5/20 4:47 AM, Doerr, Martin wrote:
> Hi Patricio,
>
> using 8 Byte load instructions for jint fields is a terrible coding style!
> Someone else may see it and use an 8 Byte store. Will result in great fun for debugging!
>
> And on Big Endian you will simply access the wrong bits.
Ah, of course! Those 32 bits will be in the wrong place when doing the test.
> Note that Big Endian Platforms are AIX, old linux ppc, s390, SPARC. I don't think you have tested on them.
>
>> We could remove the nested synchronized statements in the run() method
>> so that we don't generate that much logging. We could also lower
>> LOOP_COUNT. I think the issue is also because we are running LogTest
>> with multiple flag combinations. Not sure what we should touch first.
>> Maybe the synchronized statements, have only one or two and test that
>> first?
> Sounds like helpful ideas. Please go ahead and strip things down.
Great, I will send v3 later with those changes.
Thanks Martin!
Patricio
> Thanks for taking care of it.
>
> Best regards,
> Martin
>
>
>> -----Original Message-----
>> From: Patricio Chilano <patricio.chilano.mateo at oracle.com>
>> Sent: Dienstag, 4. August 2020 20:26
>> To: Doerr, Martin <martin.doerr at sap.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/4/20 1:35 PM, Doerr, Martin wrote:
>>> Hi Patricio,
>>>
>>>> Ok, I'll change it to movl + testl and test it out before sending v3.
>>> Thanks. I forgot to mention arm + aarch64.
>>> Aarch64 uses ldrw + tbnz.
>>> Arm 32 bit uses ldr_u32 + tbnz.
>>>
>>> I remember having seen the same mistake ��
>>> And nobody noticed it on little Endian platforms.
>> Ok, I can use a tbnz instead of test and then a branch on arm32 and aarch64.
>> Shouldn't a normal ldr in arm32 work fine?
>> Also in 64 bits (either x64 or aarch64) I don't see the issue of using a
>> 64 bit load, besides the fact that we only care about the first 32 bits.
>> Regardless of the endianness, aren't we masking out the upper part when
>> we do AND/TEST or if we test a bit in the 0-31 bit range? Otherwise it
>> seems it should have failed for one of those platforms in my tests.
>>
>>>> I see that some tests use @run driver/timeout=xxxx. Maybe you can
>>>> specify that and see if that fixes it? Let me know if that works and I
>>>> can add it to the test.
>>> That seems to have an effect. But now I'm not patient enough to wait for
>> the test to finish.
>>> Maybe the problem is that I'm using slow debug builds.
>>> But is there a chance to make the test quicker without losing coverage
>> significantly?
>>> I think the test is too slow this way to run it on a regular basis. We'd need to
>> spend dedicated machines for it.
>> We could remove the nested synchronized statements in the run() method
>> so that we don't generate that much logging. We could also lower
>> LOOP_COUNT. I think the issue is also because we are running LogTest
>> with multiple flag combinations. Not sure what we should touch first.
>> Maybe the synchronized statements, have only one or two and test that
>> first?
>>
>>
>> Thanks,
>> Patricio
>>> Best regards,
>>> Martin
>>>
>>>
>>>> -----Original Message-----
>>>> From: Patricio Chilano <patricio.chilano.mateo at oracle.com>
>>>> Sent: Dienstag, 4. August 2020 17:47
>>>> To: Doerr, Martin <martin.doerr at sap.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,
>>>>
>>>> Thanks for fixing PPC and taking care of s390!
>>>>
>>>> On 8/4/20 11:18 AM, Doerr, Martin wrote:
>>>>> Hi Patricio,
>>>>>
>>>>> I suggest to use movl + testl for checking the access flag as for other
>> access
>>>> flags on x64.
>>>> Ok, I'll change it to movl + testl and test it out before sending v3.
>>>>
>>>>> New version for PPC64 and s390 see below.
>>>>>
>>>>> The test SyncOnPrimitiveWrapperTest produces hs_err files as
>> expected.
>>>> However, I'm getting timeout issues:
>>>>> timed out (timeout set to 120000ms, elapsed time including timeout
>>>> handling was 122372ms)
>>>>> Can we provide more time?
>>>> I see that some tests use @run driver/timeout=xxxx. Maybe you can
>>>> specify that and see if that fixes it? Let me know if that works and I
>>>> can add it to the test.
>>>>
>>>> Thanks,
>>>> Patricio
>>>>> Best regards,
>>>>> Martin
>>>>>
>>>>>
>>>>> diff -r 77852e129401 src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp
>>>>> --- a/src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp Tue Aug 04
>>>> 10:03:57 2020 +0200
>>>>> +++ b/src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp Tue Aug 04
>>>> 16:04:31 2020 +0200
>>>>> @@ -107,8 +107,8 @@
>>>>>
>>>>> if (DiagnoseSyncOnPrimitiveWrappers != 0) {
>>>>> load_klass(Rscratch, Roop);
>>>>> - ld(Rscratch, in_bytes(Klass::access_flags_offset()), Rscratch);
>>>>> - andi(Rscratch, Rscratch, JVM_ACC_IS_BOX_CLASS);
>>>>> + lwz(Rscratch, in_bytes(Klass::access_flags_offset()), Rscratch);
>>>>> + testbitdi(CCR0, R0, Rscratch, exact_log2(JVM_ACC_IS_BOX_CLASS));
>>>>> bne(CCR0, slow_case);
>>>>> }
>>>>>
>>>>> diff -r 77852e129401 src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp
>>>>> --- a/src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp Tue Aug 04
>>>> 10:03:57 2020 +0200
>>>>> +++ b/src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp Tue Aug 04
>>>> 16:04:31 2020 +0200
>>>>> @@ -912,8 +912,8 @@
>>>>>
>>>>> if (DiagnoseSyncOnPrimitiveWrappers != 0) {
>>>>> load_klass(tmp, object);
>>>>> - ld(tmp, in_bytes(Klass::access_flags_offset()), tmp);
>>>>> - andi(tmp, tmp, JVM_ACC_IS_BOX_CLASS);
>>>>> + lwz(tmp, in_bytes(Klass::access_flags_offset()), tmp);
>>>>> + testbitdi(CCR0, R0, tmp, exact_log2(JVM_ACC_IS_BOX_CLASS));
>>>>> bne(CCR0, slow_case);
>>>>> }
>>>>>
>>>>> diff -r 77852e129401 src/hotspot/cpu/ppc/macroAssembler_ppc.cpp
>>>>> --- a/src/hotspot/cpu/ppc/macroAssembler_ppc.cpp Tue Aug 04
>>>> 10:03:57 2020 +0200
>>>>> +++ b/src/hotspot/cpu/ppc/macroAssembler_ppc.cpp Tue Aug 04
>>>> 16:04:31 2020 +0200
>>>>> @@ -2838,8 +2838,8 @@
>>>>>
>>>>> if (DiagnoseSyncOnPrimitiveWrappers != 0) {
>>>>> load_klass(temp, oop);
>>>>> - ld(temp, in_bytes(Klass::access_flags_offset()), temp);
>>>>> - andi(temp, temp, JVM_ACC_IS_BOX_CLASS);
>>>>> + lwz(temp, in_bytes(Klass::access_flags_offset()), temp);
>>>>> + testbitdi(CCR0, R0, temp, exact_log2(JVM_ACC_IS_BOX_CLASS));
>>>>> bne(CCR0, cont);
>>>>> }
>>>>>
>>>>> diff -r 77852e129401
>> src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp
>>>>> --- a/src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp Tue Aug 04
>>>> 10:03:57 2020 +0200
>>>>> +++ b/src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp Tue Aug 04
>>>> 16:04:31 2020 +0200
>>>>> @@ -91,6 +91,12 @@
>>>>> // Save object being locked into the BasicObjectLock...
>>>>> z_stg(obj, Address(disp_hdr,
>> BasicObjectLock::obj_offset_in_bytes()));
>>>>> + if (DiagnoseSyncOnPrimitiveWrappers != 0) {
>>>>> + load_klass(Z_R1_scratch, obj);
>>>>> + testbit(Address(Z_R1_scratch, Klass::access_flags_offset()),
>>>> exact_log2(JVM_ACC_IS_BOX_CLASS));
>>>>> + z_btrue(slow_case);
>>>>> + }
>>>>> +
>>>>> if (UseBiasedLocking) {
>>>>> biased_locking_enter(obj, hdr, Z_R1_scratch, Z_R0_scratch, done,
>>>> &slow_case);
>>>>> }
>>>>> diff -r 77852e129401 src/hotspot/cpu/s390/interp_masm_s390.cpp
>>>>> --- a/src/hotspot/cpu/s390/interp_masm_s390.cpp Tue Aug 04 10:03:57
>>>> 2020 +0200
>>>>> +++ b/src/hotspot/cpu/s390/interp_masm_s390.cpp Tue Aug 04
>> 16:04:31
>>>> 2020 +0200
>>>>> @@ -1000,6 +1000,12 @@
>>>>> // Load markWord from object into displaced_header.
>>>>> z_lg(displaced_header, oopDesc::mark_offset_in_bytes(), object);
>>>>>
>>>>> + if (DiagnoseSyncOnPrimitiveWrappers != 0) {
>>>>> + load_klass(Z_R1_scratch, object);
>>>>> + testbit(Address(Z_R1_scratch, Klass::access_flags_offset()),
>>>> exact_log2(JVM_ACC_IS_BOX_CLASS));
>>>>> + z_btrue(slow_case);
>>>>> + }
>>>>> +
>>>>> if (UseBiasedLocking) {
>>>>> biased_locking_enter(object, displaced_header, Z_R1, Z_R0, done,
>>>> &slow_case);
>>>>> }
>>>>> diff -r 77852e129401 src/hotspot/cpu/s390/macroAssembler_s390.cpp
>>>>> --- a/src/hotspot/cpu/s390/macroAssembler_s390.cpp Tue Aug 04
>>>> 10:03:57 2020 +0200
>>>>> +++ b/src/hotspot/cpu/s390/macroAssembler_s390.cpp Tue Aug 04
>>>> 16:04:31 2020 +0200
>>>>> @@ -3358,6 +3358,12 @@
>>>>> // Load markWord from oop into mark.
>>>>> z_lg(displacedHeader, 0, oop);
>>>>>
>>>>> + 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);
>>>>> + }
>>>>> +
>>>>> if (try_bias) {
>>>>> biased_locking_enter(oop, displacedHeader, temp, Z_R0, done);
>>>>> }
>>>>>
More information about the hotspot-runtime-dev
mailing list