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

[Per Liden]

Hi Coleen,

On 04/24/2019 01:18 PM, coleen.phillimore at oracle.com wrote:
> 
> 
> On 4/24/19 3:57 AM, Per Liden wrote:
>> Hi Coleen,
>>
>> I like this clean up. However, one thing that always bothered me is 
>> this pattern:
> 
> Hi Per,  Erik said you would like it.
>>
>>   MonitorLockerEx ml(&_monitor, Monitor::_no_safepoint_check_flag);
>>   ...
>>   ml.wait(Monitor::_no_safepoint_check_flag);
>>
>> And with your patch it becomes:
>>
>>   MonitorLocker ml(&_monitor, Monitor::_no_safepoint_check_flag);
>>   ...
>>   ml.wait_without_safepoint_check();
>>
>> Now, it seems a bit silly, and potentially error prone, to always have 
>> to say "don't safepoint check" twice. We already told the 
>> MonitorLocker what to do when it was constructed, so can't we just 
>> have this:
>>
>>   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?
> 
> 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.

I've also noticed that there's a slit in the code, some use the lock 
directly for wait/notify, and some use them via the MonitorLocker. I see 
a value in keeping MonitorLocker, especially if we can leave out the 
safepoint_check argument in wait(). Using the MonitorLocker tends to 
simplify code IMHO.

cheers,
Per

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