RFR: 8320318: ObjectMonitor Responsible thread [v3]

[David Holmes]

On Wed, 18 Sep 2024 18:29:23 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 two, after the review

I've taken another pass through and have a few queries.

Thanks

src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp line 535:

> 533: 
> 534:     // Set owner to null.
> 535:     // Release to satisfy the JMM

Can I suggest you use this comment form in each of the cpu-specific files. At the moment code that does the same thing is commented slightly differently with regard to the need for a release-store. Thanks.

src/hotspot/cpu/ppc/macroAssembler_ppc.cpp line 2734:

> 2732:   // We need a full fence after clearing owner to avoid stranding.
> 2733:   // StoreLoad achieves this.
> 2734:   membar(StoreLoad);

Suggestion:

  fence();

similar to S390

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

> 497: #endif
> 498: 
> 499:   // Intentional fall-through into slow path

I don't think this comment makes sense / applies any more.

src/hotspot/share/runtime/javaThread.hpp line 467:

> 465:   intx _held_monitor_count;  // used by continuations for fast lock detection
> 466:   intx _jni_monitor_count;
> 467:   ObjectMonitor* _unlocked_inflated_monitor;

At the time we store this the OM is in-use but we have unlocked it and so by the time we go to re-lock it later it may no longer be in-use. What prevents it from being deflated and deallocated? Does it require a safepoint that can't happen on that code path? If so we should add a comment to that affect somewhere.

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->dec_held_monitor_count();

So this decrement is pairing with the actual unlock that was done in the C2 code?

test/micro/org/openjdk/bench/vm/lang/LockUnlock.java line 315:

> 313:      * inflated.
> 314:      */
> 315:     @Threads(3)

Please explain this change and update the comment.

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

PR Review: https://git.openjdk.org/jdk/pull/19454#pullrequestreview-2317264716
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1767981547
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1767987462
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1767992448
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1767995887
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1768007118
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1768007650