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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Thu Apr 25 15:33:29 UTC 2019 


On 4/25/19 10:27 AM, Daniel D. Daugherty wrote:
> On 4/24/19 7:57 AM, coleen.phillimore at oracle.com wrote:
>>
>> 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 
>>
>
> src/hotspot/share/runtime/mutexLocker.hpp
>     No comments.
>
> src/hotspot/share/runtime/mutex.hpp
>     L237:    void notify ();
>     L239:    bool wait (long timeout, bool as_suspend_equivalent);
>         nit - Please delete the space before '('.

Okay.  Got it.
>
> src/hotspot/share/runtime/mutex.cpp
>     No comments.
>
> src/hotspot/share/compiler/compileBroker.cpp
> src/hotspot/share/gc/g1/g1StringDedupQueue.cpp
> src/hotspot/share/gc/shared/owstTaskTerminator.cpp
> src/hotspot/share/gc/shared/suspendibleThreadSet.cpp
> src/hotspot/share/gc/shared/workgroup.cpp
> src/hotspot/share/gc/z/zMessagePort.inline.hpp
> src/hotspot/share/gc/z/zMetronome.cpp
> src/hotspot/share/gc/z/zRuntimeWorkers.cpp
> src/hotspot/share/gc/z/zWorkers.cpp
> src/hotspot/share/runtime/init.cpp
> src/hotspot/share/runtime/serviceThread.cpp
>     No comments.
>
> Thumbs up. No need for a new webrev if you fix the space issue above.
>

Thanks for reviewing, Dan!
Coleen
> Dan
>
>
>
>>
>> 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