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

Tue Apr 30 01:23:31 UTC 2019

> I was expecting the constructor to check the name. 

Ok, this is a good idea.   I'm testing this through mach5 now but it 
passes the safepoint tests.

http://cr.openjdk.java.net/~coleenp/2019/8074355.05/webrev

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