RFR: 8320318: ObjectMonitor Responsible thread [v2]

[Coleen Phillimore]

On Fri, 13 Sep 2024 13:19:26 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.
>
> Fredrik Bredberg has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update one, after the review

I have a few more comments and questions.  Thank you!

I was wondering if we could replace TryLock with try_lock, and other camel case renames in a new/future patch to further clean this up.

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

> 897:     }
> 898: 
> 899:     if (try_set_owner_from(DEFLATER_MARKER, current) == DEFLATER_MARKER) {

This is nice.  iirc you don't need this because TryLock cancels deflation now.  Did I get this right?

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

> 918:   // To that end, the exit() operation must have at least STST|LDST
> 919:   // "release" barrier semantics.  Specifically, there must be at least a
> 920:   // STST|LDST barrier in exit() before the ST of null into _owner that drops

This sentence: Is the membar before or after the ST that drops the lock?

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

> 1222:     // falls to the new owner.
> 1223:     //
> 1224:     void* owner = try_set_owner_from(nullptr, current);

Is this the same code as TryLock now?  Except a little different...  Could this call TryLock and return if the lock becomes owned by another thread, like in SharedRuntime::monitor_exit_helper() ?

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

PR Review: https://git.openjdk.org/jdk/pull/19454#pullrequestreview-2303457835
PR Comment: https://git.openjdk.org/jdk/pull/19454#issuecomment-2353438681
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1759098067
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1759085173
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1759103684