RFR (S) 8074355: make MutexLocker smarter about non-JavaThreads

David Holmes david.holmes at oracle.com
Tue Apr 30 08:07:21 UTC 2019 

Hi Robbin,

On 30/04/2019 4:40 pm, Robbin Ehn wrote:
>> Looks good - thanks!
> 
> +1, thanks.
> 
>>
>> I almost forgot to comment on the test. :) I don't quite understand 
>> the comments about needing to acquire the Heap_lock wityhout a 
>> safepoint checks, as VerifyBeforeExit is processed by the VMThread 
>> whilst at the final safepoint.
> 
> The problematic usecase is in Threads::destroy_vm().
> A JavaThread off the ThreadsList must never use safepoint checking.

Ah I see - this is not directly related to verifyBeforeExit. It's just a 
way of "pausing" the GC while we shutdown.

> I'm playing with patch which changes the check in mutex.cpp.
> 
> -    if (self->is_Java_thread()) {
> +    // Is it a JavaThread participating in the safepoint protocol.
> +    if (self->is_active_Java_thread()) {
> 
> Which fixes both Heap and Threads lock.

Yuck. Yet another lifecycle state test :( Oh well discussion for another 
time. :)

> For the record when(/if) we succeeded to only have always and never locks,
> I strongly suggest they are different types.

Not sure about that either.

Cheers,
David

> E.g. in oopStorage we can remove asserts because we get compile error 
> instead.
> 
> /Robbin
> 
>>
>> Thanks again,
>> David
>>
>>> Thanks,
>>> Coleen
>>>
>>> On 4/29/19 8:22 PM, David Holmes wrote:
>>>> Hi Coleen,
>>>>
>>>> I'll respond to a couple of things here relating to the new webrev 
>>>> and your comments below ...
>>>>
>>>> On 30/04/2019 8:21 am, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>> An updated webrev is below.
>>>>>
>>>>>
>>>>> On 4/29/19 8:11 AM, coleen.phillimore at oracle.com wrote:
>>>>>>
>>>>>>
>>>>>> On 4/28/19 8:42 PM, David Holmes wrote:
>>>>>>> Hi Coleen,
>>>>>>>
>>>>>>> On 27/04/2019 2:10 am, coleen.phillimore at oracle.com wrote:
>>>>>>>> Summary: Use safepoint_check_always/safepoint_check_never 
>>>>>>>> instead of safepoint_check_sometimes for locks that are taken by 
>>>>>>>> JavaThreads and non-JavaThreads
>>>>>>>
>>>>>>> To clarify: the safepoint_check_[always|never|sometimes] pertains 
>>>>>>> only to the behaviour of JavaThreads that use the lock, 
>>>>>>> independently of whether a lock may be used by both JavaThreads 
>>>>>>> and non-JavaThreads.
>>>>>>
>>>>>> Yes.
>>>>>>>
>>>>>>>> This patch moves all but 3 of the locks to not be 
>>>>>>>> safepoint_check_sometimes.  We have plans to fix these three.  
>>>>>>>> Also, 
>>>>>>>
>>>>>>> So have you established that the reasons these were 'sometimes' 
>>>>>>> locks no longer apply and so it is correct to change them? Or are 
>>>>>>> you relying on testing to expose any mistaken assumptions?
>>>>>>
>>>>>> Oh, I hope not.   Robbin and I have been looking at them and he 
>>>>>> thinks we can change them for the situations that they had to be 
>>>>>> sometimes locks. The Heap_lock for example, couldn't be taken with 
>>>>>> a safepoint check on the exit path.
>>>>>>
>>>>>>>
>>>>>>>> this patch allows for being explicit about safepoint checking or 
>>>>>>>> not when the thread is a non-java thread, which is something 
>>>>>>>> that Kim objected to in my first patch.
>>>>>>>
>>>>>>> I don't understand what you mean by this. NJTs can currently call 
>>>>>>> either lock() or lock_without_safepoint_check().
>>>>>>>
>>>>>>
>>>>>> My first patch added the change for NJTs to just call lock and 
>>>>>> didn't call lock_without_safepoint_check for the 
>>>>>> safepoint_check_always flags.   Now they can call either.   My 
>>>>>> first patch also made Heap_lock an always lock, which it can't be.
>>>>>>
>>>>>>
>>>>>>>> Tested with mach5 tier1-3.
>>>>>>>>
>>>>>>>> open webrev at 
>>>>>>>> http://cr.openjdk.java.net/~coleenp/2019/8074355.03/webrev
>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8074355
>>>>>>>
>>>>>>> src/hotspot/share/gc/shared/oopStorage.cpp
>>>>>>>
>>>>>>> How can these mutexes go from currently always needing safepoint 
>>>>>>> checks to now never needing them? Are they in fact never used by 
>>>>>>> JavaThreads?
>>>>>>>
>>>>>> Now, this asserts that they can't be sometimes either.  They 
>>>>>> asserted that they couldn't be "always" locks.  These locks are 
>>>>>> low level locks and should never safepoint check.
>>>>>>
>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> src/hotspot/share/runtime/mutex.hpp
>>>>>>>
>>>>>>>  95   void check_safepoint_state   (Thread* self, bool 
>>>>>>> safepoint_check) NOT_DEBUG_RETURN;
>>>>>>>
>>>>>>> Please use the same parameter name as in the implementation: 
>>>>>>> do_safepoint_check.
>>>>>>
>>>>>> Fixed.
>>>>>>
>>>>>>>
>>>>>>> 109   // Java and NonJavaThreads. When the lock is initialized 
>>>>>>> with _safepoint_check_always,
>>>>>>>  110   // that means that whenever the lock is acquired by a 
>>>>>>> JavaThread, it will verify that each
>>>>>>>  111   // acquition from a JavaThread is done with a safepoint 
>>>>>>> check.
>>>>>>>
>>>>>>> That can simplify to just:
>>>>>>>
>>>>>>> 109   // Java and NonJavaThreads. When the lock is initialized 
>>>>>>> with _safepoint_check_always,
>>>>>>> 110   // that means that whenever the lock is acquired by a 
>>>>>>> JavaThread, it will verify that
>>>>>>> 111   // it is done with a safepoint check.
>>>>>>
>>>>>> That's better and less redundant.
>>>>>>
>>>>>>>
>>>>>>> Should we then continue:
>>>>>>>
>>>>>>> 111   // it is done with a safepoint check. In corollary when the 
>>>>>>> lock
>>>>>>> 112   // is initialized with _safepoint_check_never, that means that
>>>>>>> 113   // whenever the lock is acquired by a JavaThread it will 
>>>>>>> verify
>>>>>>> 114   // that it is done without a safepoint check.
>>>>>>>
>>>>>>> ?
>>>>>>
>>>>>> I like it.  Added with some reformatting so the paragraph is same 
>>>>>> width.
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> 38   SafepointCheckRequired not_allowed = do_safepoint_check ? 
>>>>>>> _safepoint_check_never : _safepoint_check_always;
>>>>>>> 39   assert(!self->is_Java_thread() || _safepoint_check_required 
>>>>>>> != not_allowed,
>>>>>>>
>>>>>>> I found this code very difficult to understand due to some 
>>>>>>> previous choices. All of the names that start with underscore 
>>>>>>> give the illusion (to me) of being variables (or at least being 
>>>>>>> the same kind of thing) but two are enum values and one is a 
>>>>>>> field. Using this->_safepoint_check_required would at least make 
>>>>>>> it clearer which is the field.
>>>>>>
>>>>>> Ew. no. The underscore makes it clear it's a field of the class 
>>>>>> Monitor.
>>>>>
>>>>> I don't know the convention for enum values, but maybe these could 
>>>>> say Monitor::_safepoint_check_never etc instead.  That's a typical 
>>>>> convention we use even inside member functions of the class.
>>>>
>>>> Yes - thanks! That looks much clearer (though I admit enums confuse 
>>>> me in C++, in particular with named enums I would have expected to 
>>>> see SafepointCheckRequired::_safepoint_check_never [which would be 
>>>> quite unwieldy] rather than Monitor::_safepoint_check_never.)
>>>>
>>>>>>
>>>>>>>
>>>>>>>  43   // Allow NonJavaThreads to lock and wait with a safepoint 
>>>>>>> check for locks that may be shared with JavaThreads.
>>>>>>>  44   assert(self->is_Java_thread() || !do_safepoint_check || 
>>>>>>> _safepoint_check_required != _safepoint_check_never,
>>>>>>>  45          "NonJavaThreads can only check for safepoints if 
>>>>>>> shared with JavaThreads");
>>>>>>>
>>>>>>> This is very confusing: NJTs don't do safepoint checks. I think 
>>>>>>> what you mean here is that you will allow a NJT to call lock() 
>>>>>>> rather than lock_without_safepoint_check() but only if the mutex 
>>>>>>> is "shared with JavaThreads". But always/sometimes/never has 
>>>>>>> nothing to with whether the lock is shared between JTs and NJTs. 
>>>>>>> I understand that a NJT-only mutex should, by convention, be 
>>>>>>> created with _safepoint_check_never - but it really makes no 
>>>>>>> practical difference. Further, a mutex used only by JavaThreads 
>>>>>>> could in theory also be _safepoint_check_never.
>>>>>>
>>>>>> It is confusing but this found some wild use of a lock(do 
>>>>>> safepoint check) call for a lock that is now defined as 
>>>>>> safepoint_check_never. The shared lock commentary was because for 
>>>>>> a shared lock, it can be acquired with the safepoint_check 
>>>>>> parameter from a NonJava thread.
>>>>>>
>>>>>> Maybe it should say this instead:
>>>>>>
>>>>>>   // NonJavaThreads defined with safepoint_check_never should 
>>>>>> never ask to safepoint check.
>>>>>>   assert(thread->is_Java_thread() || !do_safepoint_check || 
>>>>>> _safepoint_check_required != _safepoint_check_never,
>>>>>>          "NonJavaThread should not check for safepoint");
>>>>>>
>>>>>>>
>>>>>>> 47   // Only Threads_lock, Heap_lock and SR_lock may be 
>>>>>>> safepoint_check_sometimes.
>>>>>>> 48   assert(_safepoint_check_required != 
>>>>>>> _safepoint_check_sometimes || this == Threads_lock || this == 
>>>>>>> Heap_lock ||
>>>>>>> 49          this->rank() == suspend_resume,
>>>>>>> 50          "Lock has _safepoint_check_sometimes %s", this->name());
>>>>>>>
>>>>>>> This assert belongs in the constructor, not in every lock 
>>>>>>> operation, as it depends only on the monitor instance not on the 
>>>>>>> thread doing the lock.
>>>>>>>
>>>>>>
>>>>>> You're right, that's much better!  Fixed.
>>>>>
>>>>> This isn't better because I can't check this == Threads_lock when 
>>>>> calling the constructor on Threads_lock.  This code will go away 
>>>>> when the "sometimes" locks are fixed though.
>>>>
>>>> I was expecting the constructor to check the name.
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> http://cr.openjdk.java.net/~coleenp/2019/8074355.04/webrev
>>>>>
>>>>> Sorry for not doing incremental.  The only changes were in 
>>>>> mutex.cpp/hpp.
>>>>>
>>>>> Thanks!
>>>>> Coleen
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>> ---
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Coleen
>>>>>>
>>>>>
>>>

More information about the hotspot-dev mailing list