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

Thu Jan 25 22:42:41 UTC 2024

On Tue, 23 Jan 2024 16:14:53 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 incrementally with one additional commit since the last revision:
> 
>   Fix miss in is_recursive improvement

I last reviewed v08 of this PR. This review is for v12 of the PR.
Don't forget to make a pass to update the appropriate copyright years.

Again, I only have nits or questions that need some clarifications.

src/hotspot/share/runtime/lockStack.hpp line 101:

> 99: 
> 100:   // Try recursive enter.
> 101:   // Precondition: This lock-stack must no be full.

Nit typo: s/must no be/must not be/

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

> 48: 
> 49: inline bool LockStack::is_full() const {
> 50:   return to_index(_top) == CAPACITY;

Would it be too paranoid to use `>= CAPACITY`?
Or to add an assert that the index is not greater than capacity?

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

> 90:   // lock-stack with a length of at least 2.
> 91: 
> 92:   assert(contains(o), "entries must exist");

Perhaps: s/entries must exist/at least one entry must exist/

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

> 101:     }
> 102:     if (_base[i] == o) {
> 103:       // o can only occur in one consecutive run on the lock-stack.

I'm not sure that the claim on L103 is always true. If we have a lock stack like this:

_base[end - 1] = o1;
_base[end - 2] = o2;
_base[end - 3] = o1;
_base[end - 4] = o1;

When our `o == o1` we don't have a recursive run on the top-most part of
the lock stack, but we do have one that's lower down. L103 isn't correct in
this case, but that doesn't matter because we actually care about whether
the top most run is recursive. I think L103 can be deleted and the rest of
the comment is okay.

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

> 147: 
> 148:   int end = to_index(_top);
> 149:   if (end <= 1 || _base[end - 1] != o ||  _base[end - 2] != o) {

nit extra space: s/  _base[end - 2]/ _base[end - 2]/

src/hotspot/share/runtime/objectMonitor.inline.hpp line 106:

> 104: 
> 105: inline void ObjectMonitor::set_recursions(size_t recursions) {
> 106:   assert(_recursions == 0, "must be");

Why have the `recursions` parameter if the passed value must always be zero?

Update: It looks like you might be trying to detect some out of sync count coming
from the removal of the object from the lock stack. You expect it to always be a
count value of 1 removal and if more than 1 is removed you want to assert().

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

> 564:       markWord mark = obj()->mark_acquire();
> 565:       while (mark.is_neutral()) {
> 566:         // Retry until a lock state change has been observed.  cas_set_mark() may collide with non lock bits modifications.

nit extra space: s/.  cas_set_mark()/. cas_set_mark()/

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

> 640:       } else {
> 641:         while (mark.is_fast_locked()) {
> 642:           // Retry until a lock state change has been observed.  cas_set_mark() may collide with non lock bits modifications.

nit extra space: s/. cas_set_mark()/. cas_set_mark()/

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

Marked as reviewed by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16606#pullrequestreview-1844752233
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1467005692
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1467008430
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1467011290
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1467019715
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1467021608
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1467024242
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1467028121
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1467029683