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

[Daniel D. Daugherty]

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

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.

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


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.

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

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.

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

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.

Thumbs up! If you choose to handle and of my comments above,
I don't need to see another webrev.

Dan

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