RFR (L, tedious) 8222811: Consolidate MutexLockerEx and MutexLocker
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Thu Apr 25 13:07:24 UTC 2019
Thanks, Per!
Coleen
On 4/25/19 8:33 AM, Per Liden wrote:
> Thanks Coleen! Looks good to me, and the ZGC parts too.
>
> cheers,
> Per
>
> On 2019-04-24 13:57, 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
>>
>>
>> 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