RFR: 8319797: Recursive lightweight locking: Runtime implementation

[David Holmes]

On Fri, 10 Nov 2023 12:40:03 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.

I get the gist of this but I have a number of concerns about what is getting checked and when. I have a suspicion that recursion support is making the lockstack full more often and thus invalidating the sizing of the lockStack that was based on there being no recursion support. In that case we need to bump the size. A full lockstack should be rare not something we check for first.

src/hotspot/share/runtime/lockStack.cpp line 50:

> 48:   STATIC_ASSERT(sizeof(_bad_oop_sentinel) == oopSize);
> 49:   STATIC_ASSERT(sizeof(_base[0]) == oopSize);
> 50:   STATIC_ASSERT(std::is_standard_layout<LockStack>::value);

What is this? Are we allowed to use it?

src/hotspot/share/runtime/lockStack.cpp line 82:

> 80:       if (VM_Version::supports_recursive_lightweight_locking()) {
> 81:         oop o = _base[i];
> 82:         for (;i < top - 1; i++) {

Nit: space after first `;`

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

> 27: #define SHARE_RUNTIME_LOCKSTACK_INLINE_HPP
> 28: 
> 29: #include "runtime/lockStack.hpp"

Why was this pulled out first?

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

> 393:       // Always go into runtime if the lock stack is full.
> 394:       return false;
> 395:     }

It isn't obvious that it is beneficial to check what should be a rare occurrence. Why do this?

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

> 607:         return;
> 608:       } else if (mark.is_fast_locked() && lock_stack.is_recursive(object)) {
> 609:         // This lock is recursive but unstructured exit. Just inflate the lock.

Again this seems in the wrong place - this should be a very rare case so we should not be checking it explicitly before the expected cases!

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

PR Review: https://git.openjdk.org/jdk/pull/16606#pullrequestreview-1726411316
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1390536608
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1390537026
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1390540833
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1390546734
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1390548291