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

[coleen.phillimore at oracle.com]

On 4/24/19 7:37 AM, Robbin Ehn wrote:
>>>   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);

This could be a further change, ie easy to make.   I would rather not 
add this to this already large (in terms of source files) change.  I 
originally thought we should remove MonitorLocker.  At least I don't 
think it should allow NULL.

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

We need to revise the discussion whether to require it to be explicit 
vs. implicit though.   But this is good because now we can decide what 
to do about this one: https://bugs.openjdk.java.net/browse/JDK-8074355

Thanks,
Coleen

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