RFR: 8320318: ObjectMonitor Responsible thread [v4]

[Coleen Phillimore]

On Wed, 25 Sep 2024 13:54:07 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 with a new target base due to a merge or a rebase. The pull request now contains 14 commits:
> 
>  - Update three, after the review
>  - Merge branch 'master' into 8320318_objectmon_responsible_thread
>  - Update two, after the review
>  - Update one, after the review
>  - Small fixes before the review
>  - Merge branch 'master' into 8320318_objectmon_responsible_thread
>  - Merge branch 'master' into 8320318_objectmon_responsible_thread
>  - Removed _Responsible
>  - Fixed s390
>  - Fixed legacy locking
>  - ... and 4 more: https://git.openjdk.org/jdk/compare/0f253d11...8140570f

This is really good. I have an issue with a new comment which might conflict with another reviewer.

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

> 472: 
> 473:   // Set owner to null.
> 474:   // Release to satisfy the JMM

This comment doesn't make sense to me.  The JMM is the Java Memory Model, and this just releases the lock.  The Java memory model implies memory ordering and optimizations.  The next comment about fence is more meaningful.  Did someone want this comment?

src/hotspot/share/runtime/objectMonitor.inline.hpp line 231:

> 229:   // lifetime" of the contention mark.
> 230:   assert(!_extended, "extending twice is probably a bad design");
> 231:   _monitor->add_to_contentions(1);

This looks really good.

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

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19454#pullrequestreview-2329140950
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1775738476
PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1775741125