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

[Stefan Karlsson]

On Mon, 20 Nov 2023 20:13:32 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Make classes StackObjs
>
> 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.

Removed.

> 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/

Thanks!

> 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.

? I didn't change this code ...

> 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?

This is pre-existing code that makes sure that ZGC's concurrent GC threads participate in the handshake. I think Roman added is as a preparation for Lilliput.

> 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?

I don't remember why I moved it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399761612
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399761352
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399755200
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399757210
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399760192