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

[Coleen Phillimore]

On Fri, 19 Jan 2024 08:38:07 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> src/hotspot/cpu/x86/c2_CodeStubs_x86.cpp line 120:
>> 
>>> 118:     // The owner may be anonymous and we removed the last obj entry in
>>> 119:     // the lock-stack. This loses the information about the owner.
>>> 120:     // Write the thread to the owner field so the runtime knows the owner.
>> 
>> I'm confused by this comment.  We get here if the monitor is inflated, so we didn't remove it from the lock stack.
>
> True. This comment was written when there was an explicit monitor check before the CAS that jumped to inflated. I am not sure if there is a situation where the owner is anonymous here now. 
> 
> It should be invariant that if a thread's lock stack does not contain the oop, performs an unlock/monitorexit, the monitor is inflated and the owner is not anonymous. 
> 
> At all places in the runtime when removing the oops from the lock stack the owner field is fixed.
> 
> And in the emitted code the oop is pushed back to the lock stack incase of a failed unlock. 
> 
> There may be worth keeping this, and in the slow path after the CAS failed, check if it failed because of inflation, fix the owner field and jump back to the inflated fast path without transitioning to VM.

I'm still confused by this.   I added this, because iiuc this is an invariant here?


+#ifdef ASSERT
+    Label skip;
+    __ testb(Address(monitor, OM_OFFSET_NO_MONITOR_VALUE_TAG(owner)), (int32_t) ObjectMonitor::ANONYMOUS_OWNER);
+    __ jccb(Assembler::equal, skip);
+    __ stop("owner is anonymous");
+    __ bind(skip);
+#endif
+


I don't suggest adding this but am still trying to understand this comment.

>> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1120:
>> 
>>> 1118:     xorptr(reg_rax, reg_rax);
>>> 1119:     orptr(reg_rax, Address(monitor, OM_OFFSET_NO_MONITOR_VALUE_TAG(recursions)));
>>> 1120:     jcc(Assembler::notZero, check_successor);
>> 
>> I don't know why the  LP64/!LP64 paths are different.  Do we not decrement recursions on 32 bit, and why wouldn't we?
>
> The idea was not to change the inflated unlocking in this PR. x86_32 does not handle recursions nor successor optimisation. I see no reason that they cannot be merged and just have 32bit use the 64bit logic. However the thinking was to keep that to a separate RFE.

Ok, we can have a follow up RFE to consolidate this, so we don't have the conditional.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1461886993
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1459940998