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

Daniel D. Daugherty daniel.daugherty at oracle.com
Tue Apr 30 16:20:10 UTC 2019 

On 4/29/19 9:23 PM, coleen.phillimore at oracle.com wrote:
>> I was expecting the constructor to check the name. 
>
> Ok, this is a good idea.   I'm testing this through mach5 now but it 
> passes the safepoint tests.
>
> http://cr.openjdk.java.net/~coleenp/2019/8074355.05/webrev

src/hotspot/share/gc/cms/compactibleFreeListSpace.cpp
     No comments.

src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp
     No comments.

src/hotspot/share/gc/cms/yieldingWorkgroup.cpp
     No comments.

src/hotspot/share/gc/shared/oopStorage.cpp
     L769:          "%s: active mutex requires never to safepoint 
check", _name);
     L771:          "%s: allocation mutex requires never to safepoint 
check", _name);
         "never to safepoint" reads strangely. Perhaps "never safepoint".

src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp
     No comments.

src/hotspot/share/runtime/mutex.cpp
         L50: void Monitor::lock(Thread * self) {
     old L37:   // Ensure that the Monitor requires/allows safepoint checks.
         You deleted the comment here, but you kept the comment here:

         L105: void Monitor::lock_without_safepoint_check(Thread * self) {
         L106:   // Ensure that the Monitor does not require safepoint 
checks.

         Why the difference? Looking forward, it looks like you deleted the
         comment from other place where check_safepoint_state() is now 
called.
         So it looks like L106 is the odd man out...

     L286:   return (strcmp(name, "Threads_lock") == 0 || strcmp(name, 
"Heap_lock") == 0 || rank == Mutex::suspend_resume);
         Using "rank" to determine if this is an SR_lock instead of a name
         comparison is just begging for a comment. :-)

src/hotspot/share/runtime/mutex.hpp
     No comments.

src/hotspot/share/runtime/mutexLocker.cpp
     No comments.

src/hotspot/share/runtime/vmThread.cpp
     No comments.

test/hotspot/jtreg/runtime/Shutdown/ShutdownTest.java
     L38: // the threads list.  This fix is still valid.  This code 
requires Heap_lock be aquired
         typo: s/aquired/acquired/


Thumbs up.

Dan


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

More information about the hotspot-dev mailing list