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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Wed Apr 24 11:57:39 UTC 2019


I made the change suggested by Per and it's quite nice.  I also fixed 
Mutex to not have bodies for the private functions since that's not 
required to get a compilation error if one uses them.

http://cr.openjdk.java.net/~coleenp/2019/8222811.03.incr/webrev/index.html

Running again through mach5.

Thanks,
Coleen

On 4/24/19 7:23 AM, coleen.phillimore at oracle.com wrote:
>
>
> On 4/23/19 9:48 PM, David Holmes wrote:
>> 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/
>
> I had both of these messages reversed in wait*() functions wrt. 
> never/always.  If a lock L has safepoint_check_required = always and 
> locks or waits without safepoint check, the message is relative to the 
> lock, not the erroneous call.  Inverse for never.
>
> Thanks for finding this.
>>
>> wait_without_safepoint_check is missing:
>>
>>   // timeout is in milliseconds - with zero meaning never timeout
>>   assert(timeout >= 0, "negative timeout");
>>
>
> Fixed.
>
>> ---
>>
>> mutexLocker.hpp
>>
>> 190  protected:
>> 191   Monitor* _mutex;
>>
>> Good catch on the redundant extra field in MonitorLocker!
>
> Thanks.
>>
>> 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.
>
> I tried to do this but calling Thread::current() in the constructor 
> and having a common initialize function, required including thread.hpp 
> into mutex.hpp which then make it circular. In the end the duplicated 
> code was preferrable to any tricks I could find.
>
> I also want to have a patch (I think I said this), to make the Thread 
> argument first.  I could try some more tricks to avoid this 
> duplication when/if I do that.   How does that sound?
>
> Thanks for reviewing!
> Coleen
>
>>
>> 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>



More information about the hotspot-dev mailing list