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