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

[coleen.phillimore at oracle.com]

2 replies.

On 4/29/19 8:39 PM, David Holmes wrote:
> 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.

Okay, yes, I will list the ones I changed to the bug report.
>
>>>>> 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.

Added with comma between never and a.

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