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

Tue Apr 30 00:22:30 UTC 2019

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