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

Mon Apr 29 22:38:41 UTC 2019

Hi David, We had a bit of a collision.

On 4/29/19 6:07 PM, David Holmes wrote:
> 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?

There will be in the subsequent bug report to move the "sometimes" locks 
to "always" and "never".  I haven't filed it yet.
>
>>
>>>
>>>> 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".
>

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.

>>
>>> ---
>>>
>>> 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.
See next webrev.  I don't want to remove the _ to make the edit larger.
>
>>>
>>>  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.

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