RFR 8242263: Diagnose synchronization on primitive wrappers
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Aug 10 20:32:51 UTC 2020
I forgot to chime in on the v4 review.
The link to "v4/inc" is actually a link to the full webrev so
here's the correct incremental link:
> http://cr.openjdk.java.net/~pchilanomate/8242263/v4/inc/webrev/
src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp
src/hotspot/cpu/arm/macroAssembler_arm.cpp
src/hotspot/cpu/arm/macroAssembler_arm.hpp
Thanks for fixing the indents.
src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp
src/hotspot/cpu/ppc/macroAssembler_ppc.cpp
src/hotspot/cpu/s390/macroAssembler_s390.cpp
No comments on the PPC or S390 changes.
src/hotspot/cpu/x86/macroAssembler_x86.cpp
src/hotspot/cpu/x86/macroAssembler_x86.hpp
Thanks for fixing the indents.
test/hotspot/jtreg/runtime/Monitor/SyncOnPrimitiveWrapperTest.java
No comments.
Thumbs up.
Dan
On 8/7/20 11:29 AM, Patricio Chilano wrote:
> 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 {
>>
>
More information about the hotspot-runtime-dev
mailing list