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

[coleen.phillimore at oracle.com]

Hi Dan, Thank you for tackling this code review.  I was hoping you 
wouldn't do your usual file-by-file comments.  It would have been long.

On 4/23/19 5:43 PM, Daniel D. Daugherty wrote:
> On 4/23/19 11:06 AM, 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
>
> src/hotspot/share/runtime/mutexLocker.hpp
>     L198:       assert(_mutex->rank() > Mutex::special || 
> no_safepoint_check,
>     L199:         "Mutexes with rank special or lower should not do 
> safepoint checks");
>         nit - indent on L199 needs 5 more spaces
>
>     L200-203 - this if-block is missing the usual HotSpot '{' and '}'.
>     (Not your fault, but since you're in there...)
>
>     L211:       assert(_mutex->rank() > Mutex::special || 
> no_safepoint_check,
>     L212:         "Mutexes with rank special or lower should not do 
> safepoint checks");
>         nit - indent on L211 needs 5 more spaces
>
>     L213-216 - this if-block is missing the usual HotSpot '{' and '}'.
>     (Not your fault, but since you're in there...)

Fixed all of these.  Since I've touched it, I made it follow the coding 
style.

>
> src/hotspot/share/runtime/mutex.hpp
>     L237:    // why do these need bodies?
>         To prevent any calls to Mutex::notify(), Mutex::notify_all(),
>         Mutex::wait() and Mutex::wait_without_safepoint_check() from
>         calling the superclass' version of those functions.
>

I forgot to check this, but if they're private, they shouldn't need 
bodies.  I'll remove the comment and leave them for now.


> src/hotspot/share/runtime/mutex.cpp
>     Nice detangling into Monitor::wait() and
>     Monitor::wait_without_safepoint_check().
>
Thank you!

>
> For the rest of the files, I went through the patch from bottom -> top
> and looked for anything that didn't look right. I figure other reviewers
> are looking from top -> bottom... :-)
>
> For the multi-line MutexLocker uses in 
> src/hotspot/share/runtime/vmThread.cpp
> and src/hotspot/share/memory/metaspace.cpp,
> the second line indents appear to be off by two spaces (because "Ex" 
> was removed).
> Don't know if you want to fix those or not. Your call.

You have good eyes.  I checked all of these this morning and I missed 
these files.  Thank you.
>
> src/hotspot/share/runtime/threadSMR.cpp
>
>   - 
> ThreadsSMRSupport::delete_lock()->wait(Mutex::_no_safepoint_check_flag, 
> 0,
>   - !Mutex::_as_suspend_equivalent_flag);
>   + ThreadsSMRSupport::delete_lock()->wait_without_safepoint_check();
>
>   You dropped the '!Mutex::_as_suspend_equivalent_flag' flag. I dislike
>   default values so that's why I included it. Now the reader has to
>     1) remember that there's another parameter
>     2) go find the default value for that parameter

wait_without_safepoint_check() doesn't have as_suspend_equivalent flag 
as a parameter.  That code was only used in the safepoint checking case 
for wait.   I dislike default parameters too for the same reasons.  Both 
waits take 0 as the timeout default parameter but I thought that was not 
a good thing to change.

>
> src/hotspot/share/gc/shared/workgroup.cpp
>   This change does not parse:
>
>        // The VM thread should not execute here because MutexLocker's 
> are used
>     -  // as (opposed to MutexLockerEx's).
>     +  // as (opposed to MutexLocker's).
>
>   since MutexLocker is now on both sides of the verbal comparison.
Yeah, I missed that too.  It doesn't actually make sense before either, 
since there isn't a MutexLocker nearby.

>
> In some cases, there were multi-line MutexLocker uses that you chose
> to combine into a single line, but in most cases you did not combine
> them. I couldn't discern a rhyme or reason for the choices (not even
> a line length of 80).

I combined them if the length wasn't "too long" and it didn't look like 
the whitespace was helpful.  I did both.
>
> There is a startling number of names in use for the MutexLocker
> variables. My personal preference would be for 'ml' (and 'ul'
> for the MutexUnlocker). You have an opportunity to make them all
> consistent, but that's hard to do.

By convention, ml is used a lot but sometimes you can't use it.  I don't 
want to change this.  Sometimes people pick names they like for this.
>
> Thumbs up! If you choose to handle and of my comments above,
> I don't need to see another webrev.

Thank you Dan for reviewing this!  I'll retest with edits made for 
comments above.

Coleen

>
> Dan
>
>> bug link https://bugs.openjdk.java.net/browse/JDK-8222811
>>
>> Thanks!
>> Coleen
>>
>>
>