RFR: 8320318: ObjectMonitor Responsible thread

[David Holmes]

On Wed, 29 May 2024 12:58:02 GMT, Fredrik Bredberg <fbredberg at openjdk.org> wrote:

> Removed the concept of an ObjectMonitor Responsible thread.
> 
> The reason to have an ObjectMonitor Responsible thread was to avoid threads getting stranded due to a hole in the successor protocol. This hole was there because adding the necessary memory barrier was considered too expensive some 20 years ago.
> 
> The ObjectMonitor Responsible thread code adds complexity, and doing timed parks just to avoid getting stranded is not the way forward. More info about the problems with the ObjectMonitor responsible thread can be found in [JDK-8320318](https://bugs.openjdk.org/browse/JDK-8320318).
> 
> After removing the ObjectMonitor Responsible thread we see increased performance on all supported platforms except Windows. [JDK-8339730](https://bugs.openjdk.org/browse/JDK-8339730) has been created to handle this.
> 
> Passes tier1-tier7 on supported platforms.
> x64, AArch64, Riscv64, ppc64le and s390x passes ok on the test/micro/org/openjdk/bench/vm/lang/LockUnlock.java test.
> Arm32 and Zero doesn't need any changes as far as I can tell.

I've started looking at this and to be honest I'm surprised by the extent and complexity of the changes. The problem description sounded quite simple: get rid of the notion of the Responsible thread by putting in the fence that when missing could lead to stranding. I find it very hard to map many of the actual code changes to that problem statement. And I'm very unclear about the impact on the deflation protocol that this is causing.

I think trying to look at diffs is the wrong way to analyze this change, I need to just look at the new code and try to understand the protocol - but that makes it hard to put comments into the PR. :(

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 476:

> 474:   // Release lock.
> 475:   movptr(Address(tmpReg, OM_OFFSET_NO_MONITOR_VALUE_TAG(owner)), NULL_WORD);
> 476:   membar(StoreLoad);

Why a standalone `storeload` here? This does not define a fence, nor release semantics - as per the definitions in orderAccess.hpp

src/hotspot/share/runtime/objectMonitor.cpp line 310:

> 308: 
> 309: bool ObjectMonitor::enterI_with_contention_mark(JavaThread* locking_thread, ObjectMonitorContentionMark& contention_mark) {
> 310:   // Used by ObjectSynchronizer::enter_for() to enter for another thread.

This renaming is confusing for me. The `enter_for` methods were made explicit because normally locking is always done by the current thread for the current thread - but deopt breaks that. And now it seems we have an `EnterI` that is really an `EnterI_for` ??

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

PR Review: https://git.openjdk.org/jdk/pull/19454#pullrequestreview-2294522607
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1752964988
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1752967126