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

[David Holmes]

Hi Coleen,

On 30/04/2019 8:38 am, coleen.phillimore at oracle.com wrote:
> 
> Hi David, We had a bit of a collision.

Yep so trimming ...

> On 4/29/19 6:07 PM, David Holmes wrote:
>> On 29/04/2019 10:11 pm, coleen.phillimore at oracle.com wrote:
>>>> 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?
> 
> There will be in the subsequent bug report to move the "sometimes" locks 
> to "always" and "never".  I haven't filed it yet.

Okay but that is for the three remaining "sometimes" locks - right? Not 
the many that have been changed as part of this RFE.

>>>> 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?
> 
> These asserts in OopStorage allowed these locks to be "sometimes" locks, 
> but never "always" locks.  But the locks we actually use for OopStorage 
> were always defined as "never" locks.  We don't want them to be 
> sometimes locks.

Okay.

>>>>  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.
>>
> 
> Yes, I'm adding this constraint because it seems kinda pointless to 
> declare a lock to be safepoint_check_never and then call lock() with a 
> safepoint check, even though NJTs don't participate in the safepoint 
> protocol.  I can remove this assert but it did help me find and clean up 
> an inconsistent lock, so now everything is more clear and following the 
> rules.

I don't agree but will let it drop. I do request a change to the comment 
though because it isn't the NJT that is defined with safepoint_check_never

// If defined with safepoint_check_never a NonJavaThread should never 
ask to safepoint check either.

Thanks,
David
-----

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