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

[Axel Boldt-Christmas]

On Thu, 18 Jan 2024 21:58:36 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> 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 14 additional commits since the last revision:
>> 
>>  - Remove C2HandleAnonOMOwnerStub definitions on x86.
>>  - Add MFENCE comment
>>  - Merge remote-tracking branch 'upstream_jdk/pr/16606' into JDK-8319799
>>  - Merge remote-tracking branch 'upstream_jdk/pr/16606' into JDK-8319799
>>  - Merge remote-tracking branch 'upstream_jdk/pr/16606' into JDK-8319799
>>  - Merge remote-tracking branch 'upstream_jdk/pr/16606' into JDK-8319799
>>  - Merge remote-tracking branch 'upstream_jdk/pr/16606' into JDK-8319799
>>  - top load adjustments
>>  - Merge remote-tracking branch 'upstream_jdk/pr/16606' into JDK-8319799
>>  - Fix type
>>  - ... and 4 more: https://git.openjdk.org/jdk/compare/1c7d7234...2c709241
>
> src/hotspot/cpu/x86/c2_CodeStubs_x86.cpp line 104:
> 
>> 102:     // continuation is the slow_path.
>> 103:     __ jmp(continuation());
>> 104:   }
> 
> It seems like two stubs small stubs might be better since they don't really share very much (?) rather than one with two entry points and two exit points.

That sounds like a good idea. Will see how it ends up.

> src/hotspot/cpu/x86/c2_CodeStubs_x86.cpp line 130:
> 
>> 128:     __ movptr(Address(monitor, OM_OFFSET_NO_MONITOR_VALUE_TAG(owner)), NULL_WORD);
>> 129: 
>> 130:     // Fence.
> 
> // Instead of MFENCE we use a dummy locked add of 0 to the top-of-stack.
> Can you add this comment?

Fixed.

> src/hotspot/cpu/x86/c2_CodeStubs_x86.cpp line 154:
> 
>> 152: 
>> 153: #ifdef _LP64
>> 154: int C2HandleAnonOMOwnerStub::max_size() const {
> 
> Do we need the C2HandleAnonOMOwnerStub anymore?

Correct, not for x86_32/x86_64. Removed.

> 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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1458694728
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1458693292
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1458693056
PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1458691485