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

[David Holmes]

Hi Coleen,

On 24/04/2019 8:49 am, coleen.phillimore at oracle.com wrote:
> 
> Here's a new webrev with the changes requested by Dan.
> 
> http://cr.openjdk.java.net/~coleenp/2019/8222811.02/webrev

Overall this seems fine. A few comments on the main changes:

mutex.cpp

  156 bool Monitor::wait_without_safepoint_check(long timeout) {
  157   // Make sure safepoint checking is used properly.
  158   assert(_safepoint_check_required != 
Monitor::_safepoint_check_always,
  159          "This lock should never have a safepoint check: %s", name());

The message is the wrong one: s/never/always/

wait_without_safepoint_check is missing:

   // timeout is in milliseconds - with zero meaning never timeout
   assert(timeout >= 0, "negative timeout");

---

mutexLocker.hpp

190  protected:
191   Monitor* _mutex;

Good catch on the redundant extra field in MonitorLocker!

208   MutexLocker(Monitor* mutex, Thread* thread, 
Mutex::SafepointCheckFlag flag = Mutex::_safepoint_check_flag) :

I couldn't easily see how often you used this new constructor, but 
avoiding an unnecessary call to Thread::current() is good. However the 
duplicated code in the two constructors is not good and they differ only 
in the "thread" argument use. I don't know how C++ does constructor 
chaining but the no-thread version should just call the thread-taking 
version passing Thread::current() - or fact the body into a helper. Can 
I also request that the "thread" parameter be renamed "currentThread" 
(we're far too lax when it comes to clearly identifying when a Thread* 
must be the current thread) - thanks.

Thanks,
David

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