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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Tue Apr 30 15:59:24 UTC 2019 


On 4/30/19 4:07 AM, David Holmes wrote:
> 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.

Yes.  Thank you Robbin for answering.  The VerifyBeforeExit will crash 
though if GC isn't paused there for the exiting thread.
>
>> 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.

I'm starting to be convinced this would be good, despite more code 
churn.  But we'll see how it looks.

Thanks,
Coleen
>
> 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