RFR: 8319799: Recursive lightweight locking: x86 implementation [v14]

Fri Jan 26 21:38:45 UTC 2024

On Fri, 26 Jan 2024 09:22:48 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> Implements the x86 port of JDK-8319796.
>> 
>> There are two major parts for the port implementation. The C2 part, and the part shared by the interpreter, C1 and the native call wrapper.
>> 
>> The biggest change for both parts is that we check the lock stack first and if it is a recursive lightweight [un]lock and in that case simply pop/push and finish successfully.
>> 
>> Only if the recursive lightweight [un]lock fails does it look at the mark word. 
>> 
>> For the shared part if it is an unstructured exit, the monitor is inflated or the mark word transition fails it calls into the runtime.
>> 
>> The C2 operates under a few more assumptions, that the locking is structured and balanced. This means that some checks can be elided. 
>> 
>> First this means that in C2 unlock if the obj is not on the top of the lock stack, it must be inflated. And reversely if we reach the inflated C2 unlock the obj is not on the lock stack. This second property makes it possible to avoid reading the owner (and checking if it is anonymous). Instead it can either just do an un-contended unlock by writing null to the owner, or if contention happens, simply write the thread to the owner and jump to the runtime. 
>> 
>> The x86 C2 port also has some extra oddities. 
>> 
>> The mark word read is done early as it showed better scaling in hyper-threaded scenarios on certain intel hardware, and no noticeable downside on other tested x86 hardware. 
>> 
>> The fast path is written to avoid going through conditional branches. This in combination with keeping the ZF output correct, the code does some actions eagerly, decrementing the held monitor count, popping from the lock stack. And jumps to a code stub if a slow path is required which restores the thread local state to a correct state before jumping to the runtime.
>> 
>> The contended unlock was also moved to the code stub.
>
> Axel Boldt-Christmas has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 21 additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'upstream_jdk/pr/16606' into JDK-8319799
>  - Update variable names in ad files
>  - Preload markWord unconditionally
>  - Merge remote-tracking branch 'upstream_jdk/pr/16606' into JDK-8319799
>  - Add more expressive stub continuation names
>  - Remove outdated anonymous owner fix in stub
>  - Merge remote-tracking branch 'upstream_jdk/pr/16606' into JDK-8319799
>  - Remove C2HandleAnonOMOwnerStub definitions on x86.
>  - Add MFENCE comment
>  - Merge remote-tracking branch 'upstream_jdk/pr/16606' into JDK-8319799
>  - ... and 11 more: https://git.openjdk.org/jdk/compare/414e1ba3...4d37c4b7

I did a crawl thru review of the v12 version via webrev. I only have minor
comments and some questions. I like how the lightweight locking lock
and unlock code is separated so you don't have to deal with the older
baggage.

Thumbs up.

src/hotspot/cpu/x86/c1_MacroAssembler_x86.cpp line 142:

> 140: #else
> 141:     // This relies on the implementation of lightweight_unlock knowing that it
> 142:     // will clobber its thread when using EAX.

This use of `EAX` is confusing when earlier in this function `rax` is used.

src/hotspot/cpu/x86/c2_CodeStubs_x86.cpp line 80:

> 78: }
> 79: 
> 80: void C2FastUnlockLightweightStub::emit(C2_MacroAssembler& masm) {

I like this new stub and how clean and complete it is. I can't wait to see how it
fits in with your other changes.

src/hotspot/cpu/x86/c2_CodeStubs_x86.cpp line 96:

> 94:   }
> 95: 
> 96:   { // Restore held monitor and slow path.

Perhaps: Restore held monitor count and slow path.

src/hotspot/cpu/x86/c2_CodeStubs_x86.cpp line 128:

> 126:     // Recheck successor.
> 127:     __ cmpptr(Address(monitor, OM_OFFSET_NO_MONITOR_VALUE_TAG(succ)), NULL_WORD);
> 128:     // Seen a successor after the release -> fence we have handed of the monitor

Perhaps: // Observed a successor after the release -> fence we have handed off the monitor

src/hotspot/cpu/x86/c2_CodeStubs_x86.cpp line 131:

> 129:     __ jccb(Assembler::notEqual, fix_zf_and_unlocked);
> 130: 
> 131:     // Try to relock, if it fail the monitor has been handed over

nit typo: s/it fail/it fails/

src/hotspot/cpu/x86/c2_CodeStubs_x86.cpp line 141:

> 139: 
> 140:     __ bind(fix_zf_and_unlocked);
> 141:     __ xorl(rax, rax);

Just curious: why use `xorl` here and `xorptr` on L135 above?

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 561:

> 559:                                  Metadata* method_data,
> 560:                                  bool use_rtm, bool profile_rtm) {
> 561:   assert(LockingMode != LM_LIGHTWEIGHT, "uses fast_lock_lightweight");

I don't understand the string. Perhaps: "not for lightweight locking" or
"lightweight locking should use fast_lock_lightweight".

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 614:

> 612:     testptr(objReg, objReg);
> 613:   } else {
> 614:     assert(LockingMode == LM_LEGACY, "");

Please change the string to "must be".

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 759:

> 757: 
> 758: void C2_MacroAssembler::fast_unlock(Register objReg, Register boxReg, Register tmpReg, bool use_rtm) {
> 759:   assert(LockingMode != LM_LIGHTWEIGHT, "uses fast_unlock_lightweight");

I don't understand the string. Perhaps: "not for lightweight locking" or
"lightweight locking should use fast_unlock_lightweight".

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1027:

> 1025:   jccb(Assembler::zero, zf_correct);
> 1026:   stop("Fast Lock ZF != 1");
> 1027: #endif

I love this sanity check!

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1061:

> 1059:     stub = new (Compile::current()->comp_arena()) C2FastUnlockLightweightStub(obj, mark, reg_rax, thread);
> 1060:     Compile::current()->output()->add_stub(stub);
> 1061:   }

So what happens if `stub` doesn't get generated?

src/hotspot/cpu/x86/interp_masm_x86.cpp line 1315:

> 1313: #else
> 1314:       // This relies on the implementation of lightweight_unlock knowing that it
> 1315:       // will clobber its thread when using EAX.

This use of `EAX` is confusing when earlier in this function `rax` is used.

src/hotspot/cpu/x86/macroAssembler_x86.cpp line 9930:

> 9928: // obj: the object to be unlocked
> 9929: // reg_rax: rax
> 9930: // thread: the thread, may be EAX on x86_32

This use of `EAX` is confusing when the register is really `rax` and that's
what the code below mentions.

src/hotspot/cpu/x86/macroAssembler_x86.cpp line 9978:

> 9976:     // On x86_32 we may lose the thread.
> 9977:     get_thread(thread);
> 9978:   }

In the header comment for this function we call it `EAX`.

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

Marked as reviewed by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16607#pullrequestreview-1844845841
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1468160246
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1467977169
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1467962052
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1467970901
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1467971483
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1467975520
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1467980222
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1467981510
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1467990253
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1467999992
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1468142901
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1468160513
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1468161120
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1468162020