RFR: 8320318: ObjectMonitor Responsible thread [v2]

[Patricio Chilano Mateo]

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

Changes look good to me, but I have some comments.

Thanks

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

> 356: void ObjectMonitor::enter_for_with_contention_mark(JavaThread* locking_thread, ObjectMonitorContentionMark& contention_mark) {
> 357:   // Used by LightweightSynchronizer::inflate_and_enter in deoptimization path to enter for another thread.
> 358:   bool success = ObjectMonitor::TryLock_with_contention_mark(locking_thread, contention_mark);

No need to use qualified name.

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

> 374:   }
> 375: 
> 376:   bool success = ObjectMonitor::TryLock_with_contention_mark(locking_thread, contention_mark);

No need to use qualified name.

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

> 1265:           return;
> 1266:         }
> 1267:       }

Can't we replace all this code for a call to TryLock?

src/hotspot/share/runtime/objectMonitor.hpp line 360:

> 358: 
> 359:   enum class TryLockResult { Interference = -1, HasOwner = 0, Success = 1 };
> 360:   TryLockResult  TryLock(JavaThread* current);

This CamelCase syntax is used for private methods. We should change it to try_lock now that we are calling it from SharedRuntime code. Another alternative is to keep it private and just use the already available try_enter(). That has the benefit of not having to make TryLockResult public either. If we want to skip the checks after TryLock in try_enter we could add a check_owner_already boolean.

src/hotspot/share/runtime/objectMonitor.hpp line 376:

> 374:   ObjectWaiter* DequeueWaiter();
> 375:   void      DequeueSpecificWaiter(ObjectWaiter* waiter);
> 376:   bool      TryLock_with_contention_mark(JavaThread* locking_thread, ObjectMonitorContentionMark& contention_mark);

Following the existing style this should be TryLockWithContentionMark.

src/hotspot/share/runtime/sharedRuntime.cpp line 1973:

> 1971:       // Some other thread acquired the lock (or the monitor was
> 1972:       // deflated). Either way we are done.
> 1973:       current->inc_held_monitor_count(-1);

We can just use dec_held_monitor_count().

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

PR Review: https://git.openjdk.org/jdk/pull/19454#pullrequestreview-2306836866
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1761435185
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1761435388
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1761438126
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1761422614
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1761426498
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1761265517