RFR (L, tedious) 8222811: Consolidate MutexLockerEx and MutexLocker

[Robbin Ehn]

>>   MonitorLocker ml(&_monitor, Monitor::_no_safepoint_check_flag);
>>   ...
>>   ml.wait();
>>
>> Where wait() does the right thing, i.e. use the flag we passed into the 
>> constructor. Since all of MonitorLocker will be inlined, a flag check in 
>> wait() would be constant folded, so there's no overhead to worry about here.
>>
>> Can we please have this?

I totally agree, having to supply it twice is just error prone.
If it always is constant folded, e.g. known in compile time, it could be a 
template parameter instead:
MonitorLocker<SafepointChecked> ml(&_monitor);

Also if we remove the 3 sometimes lock and only have 'always' and 'never' locks,
having the two version as different types would give you compile errors instead 
of runtime asserts.

Thanks, Robbin

> 
> So I have a follow on patch wrt MonitorLocker.  Actually I've removed it.  Erik 
> thought having a NULL monitor wait was crazy, even though we have a case of that 
> in the GC.   The code is split whether it uses the MonitorLocker local variable 
> vs. the actual lock.
> 
> The only code that consistently uses the MonitorLocker local like this is ZGC, 
> so I was going to discuss the next change with you first anyway.
> 
> I see your point about not having to say whether you're safepoint checking twice 
> though.  People I've been talking to on the project are of two minds.  One set 
> wants it always explicit, the other implicit.  I personally want consistency 
> over all else, which is why I added wait_without_safepoint_check to MonitorLocker.
> 
> I could change this with this patch.  It's easy enough to do, especially if you 
> see value in MonitorLocker, which was going to be my next discussion with you.
> 
> Thanks,
> Coleen
> 
> 
>>
>> cheers,
>> Per
>>
>> On 4/23/19 5:06 PM, coleen.phillimore at oracle.com wrote:
>>> Summary: Make MutexLocker be MutexLockerEx implementation, remove 
>>> MutexLockerEx calls.  Also added wait_without_safepoint_check().
>>>
>>> Also made safepoint_check_flag an enum, renamed MonitorLockerEx to 
>>> MonitorLocker, removed MutexUnlockerEx, and fixed formatting where affected 
>>> by this change.
>>>
>>> There's a MutexLocker constructor with a Thread parameter that would actually 
>>> be more performant if the thread is passed from the current context rather 
>>> than calling Thread::current() in Monitor::lock.  I think the Thread 
>>> parameter should be moved to first so that we can have MutexLocker look like 
>>> the Handle constructor calls, for example.  I didn't make this change in this.
>>>
>>>     MutexLocker ml(THREAD, Monitor_lock).
>>>
>>> The interesting changes are in:
>>>
>>> http://cr.openjdk.java.net/~coleenp/2019/8222811.01/webrev/src/hotspot/share/runtime/mutexLocker.hpp.udiff.html 
>>>
>>> http://cr.openjdk.java.net/~coleenp/2019/8222811.01/webrev/src/hotspot/share/runtime/mutex.hpp.udiff.html 
>>>
>>> http://cr.openjdk.java.net/~coleenp/2019/8222811.01/webrev/src/hotspot/share/runtime/mutex.cpp.udiff.html 
>>>
>>>
>>> The rest is pattern substitution.
>>>
>>> Ran mach5 tier1-6 tests.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/2019/8222811.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8222811
>>>
>>> Thanks!
>>> Coleen
>>>
>>>
>