RFR: 8320304: Refactor and simplify monitor deflation functions [v2]

[Daniel D. Daugherty]

On Mon, 20 Nov 2023 14:02:50 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> The recent rewrites to remove safepointed monitor deflation allows us to simplify the monitor deflation code a bit. The code is now guaranteed to be called from the MonitorDeflationThread, which is a JavaThread, and never from the VMThread.
>> 
>> I would like to propose two changes to the code:
>> 1) Remove all code that supports monitor deflation from other threads than the MonitorDeflationThread
>> 2) Extract out the logging code so that it is easier to see the structure of the monitor deflation code
>> 
>> I have run minimal manual of this patch. I'd like to get some feedback on the proposal before I proceed to run more in-depth testing.
>
> Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Make classes StackObjs

Thumbs up in principal. Will have to review it again after
merging with [JDK-8319137](https://bugs.openjdk.org/browse/JDK-8319137).
Also, what Mach5 testing do you plan for this patch?

src/hotspot/share/runtime/synchronizer.cpp line 146:

> 144: 
> 145:     // Must check for a safepoint/handshake and honor it.
> 146:     safepointer->block_for_safepoint("unlinking", "unlinked_count", unlinked_count);

At the top of this code block:

L108  // The in-use list head can be null during the final audit.
L109  while (m != nullptr) {

We're no longer deflating during the final audit right? So the comment
on L108 can be deleted.

src/hotspot/share/runtime/synchronizer.cpp line 1700:

> 1698:   size_t deflated_count = deflate_monitor_list(&safepointer);
> 1699: 
> 1700:   // Unlink the deflated ObjectMonitros from the in-use list.

nit typo: s/ObjectMonitros/ObjectMonitors/

src/hotspot/share/runtime/synchronizer.cpp line 1704:

> 1702:   size_t deleted_count = 0;
> 1703:   if (deflated_count > 0) {
> 1704:     ResourceMark rm;

Can add `current` to this ResourceMark.

src/hotspot/share/runtime/synchronizer.cpp line 1716:

> 1714:     // Also, we sync and desync GC threads around the handshake, so that they can
> 1715:     // safely read the mark-word and look-through to the object-monitor, without
> 1716:     // being afraid that the object-monitor is going away.

nit spelling s/object-monitor/ObjectMonitor/
in two places unless you're trying to make some subtle point with this name.

src/hotspot/share/runtime/synchronizer.cpp line 1718:

> 1716:     // being afraid that the object-monitor is going away.
> 1717:     VM_RendezvousGCThreads sync_gc;
> 1718:     VMThread::execute(&sync_gc);

This is new to me. What does "sync and desync GC threads" mean?
And what does it mean that we didn't do this before? Is this a bug fix
that should be backported to older releases?

src/hotspot/share/runtime/synchronizer.cpp line 1745:

> 1743:   GVars.stw_random = os::random();
> 1744: 
> 1745:   log.end(deflated_count, unlinked_count);

Why move this `log.end()` call after the OM_* calls and the os::random() call?

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

Marked as reviewed by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16706#pullrequestreview-1740611580
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399700040
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399706709
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399707709
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399711550
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399712708
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399721681