RFR: 8319797: Recursive lightweight locking: Runtime implementation [v6]

[Daniel D. Daugherty]

On Tue, 21 Nov 2023 14:45:23 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> Implements the runtime part of JDK-8319796.
>> The different CPU implementations are/will be created as dependent pull requests.
>> 
>> This enhancement proposes introducing the ability for LM_LIGHTWEIGHT to handle consecutive recursive monitor enter. Limiting the implementation to only consecutive monitor enters allows for more efficient emitted code which only needs to look at the two top most entires on the lock stack to determine what to do in a monitor exit.
>> 
>> A high level overview:
>>   * Locking is still performed on the mark word
>>     * Unlocked (0b01) <=> Locked (0b00)
>>   * Monitor enter on Obj with mark word Unlocked (0b01) is the same
>>     * Transition Obj's mark word Unlocked (0b01) => Locked (0b00)
>>     * Push Obj onto the lock stack
>>     * Success
>>   * Monitor enter on Obj with mark word Locked (0b00) will check the top entry on the lock stack
>>     * If top entry is Obj
>>       * Push Obj on the lock stack
>>       * Success
>>     * If top entry is not Obj
>>       * Inflate and call ObjectMonitor::enter
>>   * Monitor exit on Obj with mark word Locked (0b00) will check the two top entries on the lock stack
>>     * If just the top entry is Obj
>>       * Transition Obj's mark word Locked (0b00) => Unlocked (0b01)
>>       * Pop the entry
>>       * Success
>>     * If both entries are Obj
>>       * Pop the top entry
>>       * Success
>>     * Any other case only occurs for unstructured locking, then just inflate and call ObjectMonitor::exit
>>   * If the monitor has been inflated for object Obj which is owned by the current thread
>>     * All corresponding entries for Obj is removed from the lock stack
>>     * The monitor recursions is set to the number of removed entries - 1
>>     * The owner is changed from anonymous to the thread
>>     * The regular ObjectMonitor::action is called.
>
> Axel Boldt-Christmas has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits:
> 
>  - Merge remote-tracking branch 'upstream_jdk/pr/16603' into JDK-8319797
>  - Merge remote-tracking branch 'upstream_jdk/pr/16603' into JDK-8319797
>  - Merge remote-tracking branch 'upstream_jdk/pr/16603' into JDK-8319797
>  - Fix nit
>  - Fix comment typos
>  - 8319797: Recursive lightweight locking: Runtime implementation

I've finished my first read-thru of this patch. I've made a few comments
and I need to mull on the changes some and will do another pass.

src/hotspot/share/runtime/lockStack.inline.hpp line 127:

> 125:   int end = to_index(_top);
> 126:   if (end <= 1 || _base[end - 1] != o ||  _base[end - 2] != o) {
> 127:     // The two topmost oop does not match o.

nit typo: s/The two topmost oop does not match o./The two topmost oops do not match o.

src/hotspot/share/runtime/lockStack.inline.hpp line 144:

> 142:   for (int i = 0; i < end; i++) {
> 143:     if (_base[i] != o) {
> 144:       _base[inserted++] = _base[i];

This version of the removal algorithm stores into every `base[inserted]` memory location
even when `inserted == i` before the first instance of `o` is found and logically removed.
Granted the lock stack is only 8 elements, but storing into every memory location when
you don't need to is wasteful.

src/hotspot/share/runtime/synchronizer.cpp line 498:

> 496:                p2i(monitor->owner()), p2i(current), monitor->object()->mark_acquire().value());
> 497:         assert(!lock_stack.is_full(), "must have made room here");
> 498:       }

This is an interesting idea. I'll have to see how you test this code later on...

src/hotspot/share/runtime/synchronizer.cpp line 504:

> 502:           // Retry until a lock state change has been observed.  cas_set_mark() may collide with non lock bits modifications.
> 503:           // Try to swing into 'fast-locked' state.
> 504:           assert(!lock_stack.contains(obj()), "thread must not already hold the lock");

It looks like the indent from L502 -> L515 is too much by two spaces.
This could be a GitHub view glitch...

test/hotspot/gtest/runtime/test_lockStack.cpp line 36:

> 34:     ls._base[ls.to_index(ls._top)] = obj;
> 35:     ls._top += oopSize;
> 36: 

nit - please delete extra blank line.

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

PR Review: https://git.openjdk.org/jdk/pull/16606#pullrequestreview-1743218757
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1401262103
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1401280804
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1402448765
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1402421724
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1402461377