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

Robbin Ehn robbin.ehn at oracle.com
Tue Apr 30 06:40:26 UTC 2019 

> 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.
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.

For the record when(/if) we succeeded to only have always and never locks,
I strongly suggest they are different types.
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