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

[coleen.phillimore at oracle.com]

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.

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