RFR (L, tedious) 8222811: Consolidate MutexLockerEx and MutexLocker
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Wed Apr 24 11:23:45 UTC 2019
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