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

[Stefan Karlsson]

On Mon, 20 Nov 2023 22:22:17 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Dan's review comments
>
> 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.)

The motivation for this is that I can hide the logging code inside this object instead of passing the LogStream and timer as parameters to the various functions. This simplifies the API, IMHO.

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

OK. I don't agree with that style, but given that this is RT code I'll oblige.

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

Maybe. I think someone needs to try it out to see if the code becomes nicer with that.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399839251
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399840473
PR Review Comment: https://git.openjdk.org/jdk/pull/16706#discussion_r1399846769