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

[coleen.phillimore at oracle.com]

On 4/24/19 8:10 AM, David Holmes wrote:
> On 24/04/2019 10:02 pm, coleen.phillimore at oracle.com wrote:
>> 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.
>
> IIRC one of the suggested follow ups from the Mutex simplification was 
> to do away with Mutex and just have Monitor.

I thought we wanted to have Monitor inherit from Mutex and add in the 
wait, notify, etc.  which is something I expected when first looking at 
this.

Cleaning up this code is the gift that keeps giving.
>
> But you're right that anyone needing a MonitorLocker for wait etc can 
> never be using NULL.

There is one:

// Update the _full_collections_completed counter
// at the end of a stop-world full GC.
unsigned int GenCollectedHeap::update_full_collections_completed() {
   MonitorLocker ml(FullGCCount_lock, Mutex::_no_safepoint_check_flag);
   assert(_full_collections_completed <= _total_full_collections,
          "Can't complete more collections than were started");
   _full_collections_completed = _total_full_collections;
   ml.notify_all();
   return _full_collections_completed;
}

I think this should be fixed.   FullGCCount_lock is NULL unless CMS or 
G1, but other collectors call this.  That's the only one I found.

Thanks,
Coleen
>
> David
> -----
>
>>>
>>> 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
>>>>>>
>>>>>>
>>>>
>>