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

Tue Apr 23 22:49:19 UTC 2019

Here's a new webrev with the changes requested by Dan.

http://cr.openjdk.java.net/~coleenp/2019/8222811.02/webrev

Thanks,
Coleen

On 4/23/19 6:18 PM, coleen.phillimore at oracle.com wrote:
>
> 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
>>>
>>>
>>
>