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

[Per Liden]

Hi David,

On 8/6/19 10:06 AM, David Holmes wrote:
> Hi Per,
> 
> On 6/08/2019 5:42 pm, Per Liden wrote:
>> Hi David,
>>
>> On 8/2/19 1:03 AM, 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
>> ---------------------------------
>> As far as I recall, the macos bug is only related to the lifecycle of 
>> condvars (not mutexes). Is that correct?
> 
> Not quite. The issue is the allocation of a pthread_mutex_t at an 
> address that previously held a pthread_condvar_t.
> 
>> If so, PlatformMutex could just always own a plain pthread_mutex_t, 
>> and the PLATFORM_MONITOR_IMPL_INDIRECT stuff could be moved to 
>> PlatformMonitor to and only deal with the lifecycle of the 
>> pthread_cond_t. That way the PlatformMutex would not carry the 
>> burden/cost for working around the condvar bug, which would make 
>> PlatformMutex even more attractive as a replacement/backing for ZLocks.
>> How does that sound?
> 
> I deliberately took the simplest path to just leave the macOS workaround 
> as is, knowing it means we still pay for the condvar slot on that 
> platform. :)
> 
> Because the bug involves both types I don't think I can constrain the 
> workaround to only the PlatformMonitor class, as I'd need to control the 
> allocation in the PlatformMutex as well. I could do that with defining 
> and instantiating a different Impl type in PlatformMonitor to be 
> installed by PlatformMutex, but the overall complexity increase doesn't 
> seem like a good trade off to me. Perhaps I've missed a simple way to 
> deal with this?

Ok. So we can't have a mutex_t at an address where a cond_t has lived in 
the past. We guarantee that this can not happen by never actually 
deallocating any cond_t and instead have our own freelist, essentially 
protecting those cond_t addresses from ever being reused for any other 
type. Even if this bug involves both types, just constraining cond_t 
should be enough to solve the problem, right?

Just to illustrate what I'm thinking, I adjusted your patch:
http://cr.openjdk.java.net/~pliden/8228857/webrev.0

And here's just the diff against your patch:
http://cr.openjdk.java.net/~pliden/8228857/webrev.0-diff

This keeps the code pretty similar, but puts the cost on PlatforMonitor. 
Can you spot a problem with this?

cheers,
Per

> 
> Thanks,
> David
> 
>> cheers,
>> Per
>>
>>>
>>> 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
>>> -----