RFR (S) 8074355: make MutexLocker smarter about non-JavaThreads
Robbin Ehn
robbin.ehn at oracle.com
Tue Apr 30 10:59:29 UTC 2019
Hi Coleen,
I think you missed a spot for freeList lock:
if (is_par) _indexedFreeListParLocks[rem_sz]->lock();
returnChunkToFreeList(ffc);
split(size, rem_sz);
if (is_par) _indexedFreeListParLocks[rem_sz]->unlock();
I hit this assert:
# assert(self->is_Java_thread() || !do_safepoint_check ||
_safepoint_check_required != _safepoint_check_never) failed: NonJavaThreads can
only check for safepoints if shared with JavaThreads
Since we pass this lock around like crazy there is no way to search for this lock.
If 'never locks' was a different type from 'always locks' this would have been
caught in compile time... :)
V [libjvm.so+0x131b1ac]
FreeChunk*CompactibleFreeListSpace::splitChunkAndReturnRemainder(FreeChunk*,unsigned
long)+0x38c
V [libjvm.so+0x1319f74]
FreeChunk*CompactibleFreeListSpace::getChunkFromDictionaryExact(unsigned long)+0x2c4
V [libjvm.so+0x131d854]
HeapWordImpl**CompactibleFreeListSpaceLAB::alloc(unsigned long)+0xb4
V [libjvm.so+0x139194c] oop
ConcurrentMarkSweepGeneration::par_promote(int,oop,markOopDesc*,unsigned long)+0x18c
V [libjvm.so+0x1f68170] oop
ParNewGeneration::copy_to_survivor_space(ParScanThreadState*,oop,unsigned
long,markOopDesc*)+0x2d0
V [libjvm.so+0x1f72894] void
ParScanClosure::do_oop_work<unsigned>(__type_0*,bool,bool)+0x674
V [libjvm.so+0x1f7b91c] void
InstanceKlass::oop_oop_iterate_oop_maps<unsigned,ParScanWithBarrierClosure>(oop,__type_1*)+0x36c
V [libjvm.so+0x1f79590] void
OopOopIterateDispatch<ParScanWithBarrierClosure>::Table::oop_oop_iterate<InstanceKlass,unsigned>(ParScanWithBarrierClosure*,oop,Klass*)+0x80
V [libjvm.so+0x1f65d68] void ParEvacuateFollowersClosure::do_void()+0xbc8
V [libjvm.so+0x1f6677c] void ParNewGenTask::work(unsigned)+0x10c
V [libjvm.so+0x23c05d4] void GangWorker::loop()+0xa4
V [libjvm.so+0x2268df8] void Thread::call_run()+0x178
V [libjvm.so+0x1f37b6c] thread_native_entry+0x3ac
/Robbin
On 2019-04-30 10:07, David Holmes wrote:
> Hi Robbin,
>
> On 30/04/2019 4:40 pm, Robbin Ehn wrote:
>>> Looks good - thanks!
>>
>> +1, thanks.
>>
>>>
>>> I almost forgot to comment on the test. :) I don't quite understand the
>>> comments about needing to acquire the Heap_lock wityhout a safepoint checks,
>>> as VerifyBeforeExit is processed by the VMThread whilst at the final safepoint.
>>
>> The problematic usecase is in Threads::destroy_vm().
>> A JavaThread off the ThreadsList must never use safepoint checking.
>
> Ah I see - this is not directly related to verifyBeforeExit. It's just a way of
> "pausing" the GC while we shutdown.
>
>> I'm playing with patch which changes the check in mutex.cpp.
>>
>> - if (self->is_Java_thread()) {
>> + // Is it a JavaThread participating in the safepoint protocol.
>> + if (self->is_active_Java_thread()) {
>>
>> Which fixes both Heap and Threads lock.
>
> Yuck. Yet another lifecycle state test :( Oh well discussion for another time. :)
>
>> For the record when(/if) we succeeded to only have always and never locks,
>> I strongly suggest they are different types.
>
> Not sure about that either.
>
> Cheers,
> David
>
>> E.g. in oopStorage we can remove asserts because we get compile error instead.
>>
>> /Robbin
>>
>>>
>>> Thanks again,
>>> David
>>>
>>>> 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