RFR: 8320318: ObjectMonitor Responsible thread

Tue Sep 10 20:06:13 UTC 2024

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.

This is such a nice simplifying change.  I have some more suggestions.

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

> 480:   // This is faster on Nehalem and AMD Shanghai/Barcelona.
> 481:   // See https://blogs.oracle.com/dave/entry/instruction_selection_for_volatile_fences
> 482:   lock(); addl(Address(rsp, 0), 0);

Since there's a membar above, do you need this lock/addl instructions?

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

> 508: 
> 509:   // Memory barrier/fence
> 510:   // Dekker pivot point -- fulcrum : ST Owner; MEMBAR; LD Succ

I think you should delete this whole comment block.  The source code control system will remember this comment about Nehalem and AMD Shanghai/Barcelona.

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

> 618: 
> 619:   // Support for SharedRuntime::monitor_exit_helper()
> 620:   ObjectMonitor* unlocked_inflated_monitor() { return _unlocked_inflated_monitor; }

Can you make this a const method?

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

> 351: 
> 352: void ObjectMonitor::enter_for_with_contention_mark(JavaThread* locking_thread, ObjectMonitorContentionMark& contention_mark) {
> 353:   DEBUG_ONLY(bool success = ) ObjectMonitor::enterI_with_contention_mark(locking_thread, contention_mark);

This is kind of noisy with DEBUG_ONLY.  If you remove DEBUG_ONLY, does the windows compiler complain that you're not using the variable success in the product build?

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

> 572:   for (;;) {
> 573:     if (own == DEFLATER_MARKER) {
> 574:       if (TryLockI(current)) {

I can't tell the difference between TryLockI and enter_for().  Did I previously object to enter_for() here?  Maybe I should take that back, and there should be a comment above enter_for() like
     // Enters a lock in behalf of a non-current thread, or a thread that is exiting and has previously given up the lock.
     // and it handles deflation.

You could add a boolean that you expect success for the enter_for() caller from deoptimization (ie. must_succeed).

This code is getting repetitive - it looks the same in all these places only a little bit different and hard to know why.

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

> 586:       } else {
> 587:         // The lock had been free momentarily, but we lost the race to the lock.
> 588:         own = prev_own;

So this retries now and doesn't break.  Is it because it could be the DEFLATER_MARKER ?

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

> 902:   }
> 903: 
> 904:   assert(_succ != current, "invariant");

This assert seems unnecessary since it's just reset above.

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

> 1102: //   1. A release barrier ensures that changes to monitor meta-data
> 1103: //      (_succ, _EntryList, _cxq) and data protected by the lock will be
> 1104: //      visible before we release the lock.

Where is this barrier?

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

> 1241:       ObjectMonitorContentionMark contention_mark(this);
> 1242: 
> 1243:       if (contentions() < 0) {

You should use is_being_async_deflated() here instead of contentions() < 0.

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

> 1242: 
> 1243:       if (contentions() < 0) {
> 1244:         assert((intptr_t(_EntryList)|intptr_t(_cxq)) == 0 || _succ != nullptr, "");

Please add a space between | in this expression.

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

Changes requested by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19454#pullrequestreview-2292693079
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1752089574
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1752087824
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1752384806
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1752639893
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1752655374
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1752397722
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1752403154
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1752408986
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1752432910
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1752435904