RFR (S) 8074355: make MutexLocker smarter about non-JavaThreads
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Apr 30 15:49:54 UTC 2019
On 4/30/19 6:59 AM, Robbin Ehn wrote:
> 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();
Got it. I was able to reproduce this and test the fix. Thank you!
>
> 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... :)
Okay, you're made this more convincing. I'll open an RFE and we can
discuss this.
Thanks for the review!
Coleen
>
> 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