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

[David Holmes]

On Mon, 20 Nov 2023 21:30:47 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:
> 
>   Dan's review comments

Okay in principle but I found it hard to see  the overall effect of the changes. Abstracting out the logging is good, but the other refactoring didn't really work for me. I have some suggestions on the coupling between this code and the `MonitorDeflationThread` code. Thanks.

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

> 87: }
> 88: 
> 89: class ObjectMonitorDeflationSafepointer : public StackObj {

FWIW I'm really not seeing the value in abstracting this out into a helper and then passing that object through the API versus simply using a static method like the old `ObjectSynchronizer::chk_for_block_req`. I guess it is a matter of personal style/preference, but I  found it harder to spot where the safepoint checking was actually happening. (Just a comment not a request for change.)

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

> 93: public:
> 94:   ObjectMonitorDeflationSafepointer(ObjectMonitorDeflationLogging* log)
> 95:     : _current(JavaThread::current()), _log(log) {}

The code that is using this already has a reference to the current thread so we should pass that through, not rematerialize it through `JavaThread::current()`. Thanks.

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

> 1684: size_t ObjectSynchronizer::deflate_idle_monitors() {
> 1685:   JavaThread* current = JavaThread::current();
> 1686:   assert(current->is_monitor_deflation_thread(), "The only monitor deflater");

Given we know the thread that has to be involved here I'm wondering if we can couple the `MonitorDeflationThread` more tightly to this code in general. E.g. make this private and make the `MonitorDeflationThread` class a friend. Store the instance and have an accessor like we do for VMThread and avoid any need to materialize `JavaThread::current()`? You could even move the MDT code into a nested class to show the tight coupling.

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

PR Review: https://git.openjdk.org/jdk/pull/16706#pullrequestreview-1739168442
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399821607
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1398793549
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399804745