RFR (S): 8228857: Refactor PlatformMonitor into PlatformMutex and PlatformMonitor

[David Holmes]

Hi Dan,

Sorry I forgot to reply to you - thanks for looking at this.

On 8/08/2019 1:37 am, Daniel D. Daugherty wrote:
> On 8/1/19 7:03 PM, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8228857
>> webrev: http://cr.openjdk.java.net/~dholmes/8228857/webrev/
> 
> src/hotspot/os/posix/os_posix.hpp
>      L245: // Platform specific implementations that underpin VM 
> Monitor/Mutex classes
>          Perhaps "Mutex/Monitor" instead of "Monitor/Mutex" now that
>          Mutex is the base class. And it will be consistent with the
>          new comments that you've added.

Changed.

> src/hotspot/os/posix/os_posix.inline.hpp
>      No comments.
> 
> src/hotspot/os/posix/os_posix.cpp
>      L2277: // Platform Monitor implementation
>          Perhaps: // Platform Mutex/Monitor implementation
>          like you do in other places.

Changed.

> src/hotspot/os/solaris/os_solaris.hpp
>      L338: // Platform specific implementations that underpin VM 
> Monitor/Mutex classes
>          Perhaps "Mutex/Monitor" instead of "Monitor/Mutex" now that
>          Mutex is the base class.

Changed.

> src/hotspot/os/solaris/os_solaris.cpp
>      No comments.
> 
> src/hotspot/os/windows/os_windows.hpp
>      L190: // Platform specific implementations that underpin VM 
> Monitor/Mutex classes
>          Perhaps "Mutex/Monitor" instead of "Monitor/Mutex" now that
>          Mutex is the base class.

Changed.

> src/hotspot/os/windows/os_windows.inline.hpp
>      L98: inline os::PlatformMonitor::~PlatformMonitor() {
>          I don't remember why there is no 'DeleteConditionVariable()'
>          call here. It might be worth a comment to remind the reader.

Comment added - there is no DeleteConditionVariable function. :)

> Thumbs up. If you choose to make the comment tweaks above, I don't
> need to see another webrev.

v2 coming real soon due to the macOS stuff.

Thanks,
David

> Dan
> 
> 
> 
>>
>> As suggested by Per Liden when PlatformMonitor was introduced by 
>> JDK-8210832 we should refactor PlatformMonitor into a simpler 
>> PlatformMutex extended by PlatformMonitor. That would potentially 
>> allow other synchronization objects e.g. Zlocks, to be replaced by a 
>> suitable PlatformX class, and allows us to redefine JVM_RawMonitor 
>> support to use it (forthcoming change).
>>
>> The refactoring would be obvious and simple if not for the macOS 
>> PThread allocation bug workaround. I had to change the Posix variant 
>> so that we only use the Impl helper class on macOS, so that we can 
>> separate the allocation of the pthread_mutex_t and pthread_cond_t. For 
>> macOS both get allocated in the PlatformMutex.
>>
>> There are not actual usage changes in this RFE.
>>
>> Testing: mach5 tiers 1-3
>>
>> Thanks,
>> David
>> -----
>>
>