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

[David Holmes]

Hi Coleen,

On 29/04/2019 10:11 pm, 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.

So is there a write up of this analysis for the bug report?

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

So these locks are only used by JavaThreads but never with a safepoint 
check? Thats perfectly valid - I just find it very strange they were 
previously marked as "always".

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

But that is the problem, the underscore does not make it clear it is a 
field because _safepoint_check_never and _safepoint_check_always are not 
fields they are enum values! I think we need a convention for enums 
values to ensure they can't be mistaken for fields. But that's another 
issue ...

Aside: the hotspot style guide says to avoid use of leading underscores.

>>
>>  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");

Whether NJTs use lock() or lock_without_safepoint_check() they are never 
"asking for a safepoint check" because it is simply doesn't apply to 
them. You seem to be wanting to enforce that for "never" locks NJTs must 
use lock_without_safepoint_check() - but that seems an arbitrary 
constraint. They can use lock() as well because the safepoint-check part 
is irrelevant. Otherwise you should be barring NJTs from being able to 
call lock() in all cases regardless of whether it is a 
always/sometimes/never lock.

Thanks,
David
-----

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