RFR (S) 8074355: make MutexLocker smarter about non-JavaThreads
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Apr 30 18:47:17 UTC 2019
Thank you for reviewing this, Dan!
On 4/30/19 12:20 PM, Daniel D. Daugherty wrote:
> 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".
Ok, fixed, took out both "to".
>
> 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...
I deleted the comment at L:106 to be consistent because the function
also has comments.
>
> 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. :-)
I originally used "rank" because because in the old location, I was
couldn't compare this == SR_lock because there are more than one. Since
it uses name in the constructor for the others, I changed it to this:
// Only Threads_lock, Heap_lock and SR_lock may be
safepoint_check_sometimes.
bool is_sometimes_ok(const char* name) {
return (strcmp(name, "Threads_lock") == 0 || strcmp(name,
"Heap_lock") == 0 || strcmp(name, "SR_lock") == 0);
}
>
> 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/
>
Fixed. I have fixed the test for Leonid also, so I'll send out another
version. Sorry for not sending out incremental webrevs. I should have
used mq.
Thanks!
Coleen
>
> 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