RFR (L, tedious) 8222811: Consolidate MutexLockerEx and MutexLocker
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Wed Apr 24 12:49:18 UTC 2019
On 4/24/19 8:14 AM, David Holmes wrote:
> On 24/04/2019 9:23 pm, 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.
>
> That's a shame. If Kim were around he might be able to suggest a trick
> :) I don't suppose something like:
>
> extern Thread * Thread::current();
>
> is valid? :)
I don't know. This makes me uneasy too. Maybe we can think of
something when he gets back.
>
>> 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?
>
> Sounds good.
Thanks!
Coleen
>
> Thanks,
> David
> -----
>
>> 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