RFR: 8319947: Recursive lightweight locking: s390x implementation

[Axel Boldt-Christmas]

On Tue, 21 May 2024 11:42:28 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> s390x port for recursive locking. 
>> 
>> testing:
>> - [x] build fastdebug-vm
>> - [x] build slowdebug-vm
>> - [x] build release-vm
>> - [x] build optimized-vm
>> - [x] ./test/jdk/java/util/concurrent (fastdebug-vm)
>>   - [x] with C1
>>   - [x] with C2
>>   - [x] with interpreter 
>> - [x] ./test/jdk/java/util/concurrent (release-vm)
>>   - [x] with C1
>>   - [x] with C2
>>   - [x] with interpreter 
>> - [x] ./test/jdk/java/util/concurrent (slowdebug-vm)
>>   - [x] with C1
>>   - [x] with C2
>>   - [x] with interpreter
>> - [x] tier1 with fastdebug-vm 
>> - [x] tier1 with slowdebug-vm
>> - [x] tier1 with release-vm
>> 
>> *BenchMarks*:
>> 
>> Results from Performance LPARs : 
>> 
>> 
>> Locking Mode = 1 (without Patch)
>> 
>> Benchmark                                (innerCount)  Mode  Cnt     Score    Error  Units
>> LockUnlock.testContendedLock                      100  avgt   12     5.144 ±  0.035  ns/op
>> LockUnlock.testRecursiveLockUnlock                100  avgt   12  3824.742 ± 89.475  ns/op
>> LockUnlock.testRecursiveSynchronization           100  avgt   12    25.348 ±  0.559  ns/op
>> LockUnlock.testSerialLockUnlock                   100  avgt   12   466.629 ±  3.036  ns/op
>> LockUnlock.testSimpleLockUnlock                   100  avgt   12   468.532 ±  1.793  ns/op
>> Finished running test 'micro:vm.lang.LockUnlock'
>> 
>> Locking Mode = 1 (with patch)
>> 
>> Benchmark                                (innerCount)  Mode  Cnt     Score    Error  Units
>> LockUnlock.testContendedLock                      100  avgt   12     5.146 ±  0.027  ns/op
>> LockUnlock.testRecursiveLockUnlock                100  avgt   12  3833.175 ± 75.863  ns/op
>> LockUnlock.testRecursiveSynchronization           100  avgt   12    25.206 ±  0.519  ns/op
>> LockUnlock.testSerialLockUnlock                   100  avgt   12   473.973 ±  2.103  ns/op
>> LockUnlock.testSimpleLockUnlock                   100  avgt   12   470.749 ±  2.229  ns/op
>> Finished running test 'micro:vm.lang.LockUnlock'
>> 
>> 
>> 
>> 
>> Locking Mode = 2  (without Patch)
>> 
>> Benchmark                                (innerCount)  Mode  Cnt      Score    Error  Units
>> LockUnlock.testContendedLock                      100  avgt   12      4.688 ±  0.051  ns/op
>> LockUnlock.testRecursiveLockUnlock                100  avgt   12  12800.544 ± 92.265  ns/op
>> LockUnlock.testRecursiveSynchronization           100  avgt   12     26.486 ±  2.229  ns/op
>> LockUnlock.testSerialLockUnlock                   100  avgt   12    424.499 ±  0.416  ns/op
>> LockUnlock.te...
>
> src/hotspot/cpu/s390/interp_masm_s390.cpp line 1013:
> 
>> 1011:     assert((JVM_ACC_IS_VALUE_BASED_CLASS & 0xFFFF) == 0, "or change following instruction");
>> 1012:     z_nilh(tmp, JVM_ACC_IS_VALUE_BASED_CLASS >> 16);
>> 1013:     z_brne(slow_case);
> 
> Is this change unrelated to recursive lightweight? If so should it be a separate RFE?

I see you implemented it in `compiler_fast_lock_lightweight_object` and changed it in `compiler_fast_lock_object` as well. Maybe that answers my question.

> src/hotspot/cpu/s390/macroAssembler_s390.cpp line 5757:
> 
>> 5755: 
>> 5756:   z_csg(mark, top, oopDesc::mark_offset_in_bytes(), obj);
>> 5757:   branch_optimized(Assembler::bcondNotEqual, slow);
> 
> The previous comment and spacing makes this a little harder for me to read and understand. 
> ```C++
>   // Try to lock. Transition lock-bits 0b01 => 0b00
>   z_oill(mark, markWord::unlocked_value);
>   z_lgr(top, mark);
>   z_xilf(top, markWord::unlocked_value);
>   z_csg(mark, top, oopDesc::mark_offset_in_bytes(), obj);
>   branch_optimized(Assembler::bcondNotEqual, slow);
> 
> If comments to clarify the `mark |= 1; top = mark; top ^= 1;` logic requires a comment, it can be added but without the newlines.

I saw below that you do it like this with the `    // Clear lock-bits from locked_obj (locked state)` comment added in `compiler_fast_lock_lightweight_object`. Maybe have it the same way here.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18878#discussion_r1608212819
PR Review Comment: https://git.openjdk.org/jdk/pull/18878#discussion_r1608210500