8203341: Add a safepoint-aware Semaphore
Stefan Karlsson
stefan.karlsson at oracle.com
Tue May 22 06:45:53 UTC 2018
Hi David,
Thanks for reviewing and for the help to get this sorted out. I'll push
the proposed webrev.04.delta patch.
StefanK
On 2018-05-22 03:19, David Holmes wrote:
> Hi Stefan,
>
> On 22/05/2018 12:42 AM, Stefan Karlsson wrote:
>> Hi David,
>>
>> On 2018-05-21 02:35, David Holmes wrote:
>>> Hi Stefan,
>>>
>>> This updated webrev only came out late Friday night for me so I did
>>> not get a chance to review before you pushed it.
>>
>> Let's continue the discussion here, and push an update to the code as
>> a new RFE.
>>
>>> I would have argued for keeping consistency with existing similar
>>> API's and use the "bool safepoint_check" parameter instead of adding
>>> a new variant of the "wait" method.
>>>
>>> src/hotspot/share/runtime/semaphore.inline.hpp
>>>
>>> 2 * Copyright (c) 2015, 2018,
>>>
>>> How does a new file have a 2015 copyright?
>>
>> This code comes from the ZGC repository, and was created 2015.
>>
>>>
>>> 41 inline void Semaphore::wait_with_safepoint_check() {
>>> 42 Thread* thread = Thread::current();
>>> 43 if (thread->is_Java_thread()) {
>>> 44 wait_with_safepoint_check(static_cast<JavaThread*>(thread));
>>> 45 } else {
>>> 46 // Wait for value
>>> 47 _impl.wait();
>>> 48 }
>>> 49 }
>>>
>>> This seems misnamed - if not a JavaThread then there is no safepoint
>>> check.
>>>
>>> It seems confusing to me to now have three wait variants with no
>>> clear indicator as to whether specific types of threads should only
>>> call specific variants (and no assertions checking that). I would
>>> expect JavaThreads to always call
>>> wait_with_safepoint_check(JavaThread), while non-JavaThread's should
>>> only call wait(). The parameter-less wait_with_safepoint_check()
>>> doesn't seem desirable. I'm guessing it is there to allow the
>>> dispatching to be done by the semaphore code rather than having the
>>> callers call Thread::current() and dispatch themselves?
>>
>> Yes, this is what the code in ZGC looks like:
>> http://hg.openjdk.java.net/zgc/zgc/file/d99bcf9de8f3/src/hotspot/share/gc/z/zFuture.inline.hpp
>>
>>
>> This code can be called from JavaThreads and GC threads
>> (non-JavaThreads), and after this change this would start to use
>> wait_with_safepoint_check().
>>
>>>
>>> But unless we may sometimes want JavaThreads to not perform safepoint
>>> checks (do we??),
>>> this seems overly complicated. We could have simply defined wait to
>>> take a Thread parameter (default NULL) and then decide whether to use
>>> ThreadBlockInVM or not based on whether it was a JavaThread.
>>
>> This would introduce a virtual call in the handshake code.
>>
>>> At most two variants seem needed (if you don't like potentially
>>> calling Thread::current() if you don't need the Thread) rather than
>>> three.
>>
>> We have these use-cases (maybe there are more):
>> 1) GC threads using Semaphores - using Semaphore::wait - no safepoint
>> checking
>> 2) Java threads executing handshake code -
>> Semaphore::wait_with_safepoint_check(JavaThread*) - with safepoint
>> checking
>> 3) ZGC code that are executed by both GC threads and Java threads -
>> using Semaphore::wait_with_safepoint_check() - dispatching to either
>> (1) or (2).
>>
>> We could easily move the code in
>> Semaphore::wait_with_safepoint_check() to ZGC, if you don't think it's
>> usable by other parts of the VM. That would also solve the naming
>> problem.
>>
>> http://cr.openjdk.java.net/~stefank/8203341/webrev.04.delta/
>> http://cr.openjdk.java.net/~stefank/8203341/webrev.04/
>>
>> Is this OK, or do you still want a bool to mimic the Mutex API? I
>> think most usages of Semaphores want to use the wait() function
>> (without safepoint check), but the default for Mutex is to lock with
>> safepoint check.
>
> tl;dr The above looks okay.
>
> To be clear ... JavaThreads always want a safepoint check;
> non-JavaThreads never want a safepoint check, so it boils down to how
> you decide which variant to call.
>
> On the callee side you can either have two methods with different
> semantics (as proposed) or one method that takes a parameter to select
> the behaviour (as mutex does). Given the pre-existence of mutex et al I
> prefer to continue using a parameter to select, but can live with two
> methods.
>
> Then at the caller side it comes down to whether you know if you are a
> JavaThread or not. If you know you are and you have an existing thread
> reference, or you know you are not, then you can call the right variant
> directly. Otherwise you need to obtain the currentThread and/or check
> its type - that can either be done at the callsite or folded inside the
> argument-less dispatching wait_xxx() method as per the committed
> changes. If you go with the argument-less dispatching variant then
> accurate naming becomes awkward: wait_with_safepoint_check_if_needed().
> Blegghh. :)
>
> So I prefer the callsites doing the dispatching themselves as an initial
> position - and if we find this becomes burdensome then consider changing
> it.
>
> So what you propose in webrev.04 seems fine to move this along.
>
> Thanks,
> David
>
>>
>> Thanks,
>> StefanK
>>
>>>
>>> Cheers,
>>> David
>>> -----
>>>
>>> On 18/05/2018 10:45 PM, Stefan Karlsson wrote:
>>>> I got offline suggestions that I should rename the function to
>>>> wait_with_safepoint_check:
>>>>
>>>> http://cr.openjdk.java.net/~stefank/8203341/webrev.03/
>>>> http://cr.openjdk.java.net/~stefank/8203341/webrev.03.delta/
>>>>
>>>> Thanks,
>>>> StefanK
>>>>
>>>> On 2018-05-18 14:21, Erik Österlund wrote:
>>>>> Hi Stefan,
>>>>>
>>>>> Looks good.
>>>>>
>>>>> Thanks,
>>>>> /Erik
>>>>>
>>>>> On 2018-05-18 14:13, Stefan Karlsson wrote:
>>>>>> Updated webrevs:
>>>>>> http://cr.openjdk.java.net/~stefank/8203341/webrev.02/
>>>>>> http://cr.openjdk.java.net/~stefank/8203341/webrev.02.delta/
>>>>>>
>>>>>> I removed the SemaphoreSafepointAware class, as Erik requested,
>>>>>> but instead of adding a bool I added a new function for the
>>>>>> safepoint aware waits.
>>>>>>
>>>>>> Thanks,
>>>>>> StefanK
>>>>>>
>>>>>> On 2018-05-17 17:59, Stefan Karlsson wrote:
>>>>>>> On 2018-05-17 17:38, Erik Österlund wrote:
>>>>>>>> Hi Stefan,
>>>>>>>>
>>>>>>>> I wonder if it would make sense to let Semaphore::wait have a
>>>>>>>> bool safepoint_check, similar to Mutex (possibly with a default
>>>>>>>> value), instead of introducing a new class with the same API,
>>>>>>>> that only differs in this property.
>>>>>>>
>>>>>>> That sounds reasonable to me.
>>>>>>>
>>>>>>> I also need to change my patch so that the thread-local handshake
>>>>>>> code pass in the existing JavaThread, so that we don't have to
>>>>>>> call Thread::current()->is_JavaThread() unnecessarily.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> StefanK
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> /Erik
>>>>>>>>
>>>>>>>> On 2018-05-17 09:46, Stefan Karlsson wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> Please review this patch to add a safepoint-aware Semaphore
>>>>>>>>> (and refactor the semaphore usage in thread-local handshakes).
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~stefank/8203341/webrev.01/
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8203341
>>>>>>>>>
>>>>>>>>> Both ZGC and the thread-local handshakes need to move the
>>>>>>>>> JavaThread to a blocked state before waiting on a Semaphore. I
>>>>>>>>> propose that we introduce a new SemaphoreSafepointAware wrapper
>>>>>>>>> around Semaphore.
>>>>>>>>>
>>>>>>>>> There are two things to consider with this patch:
>>>>>>>>>
>>>>>>>>> 1) In ZGC, where this patch originates from, we set the
>>>>>>>>> JavaThread's osthread state with
>>>>>>>>> osthread->set_state(CONDVAR_WAIT) (using OSThreadWaitState).
>>>>>>>>> This means that we get the following printed when the threads
>>>>>>>>> are dumped: "waiting on condition ". I've kept this behavior
>>>>>>>>> for the SemaphoreSafepointAware class. This means that we now
>>>>>>>>> change the osthread state for JavaThreads blocking in
>>>>>>>>> HandshakeState::process_self_inner.
>>>>>>>>>
>>>>>>>>> 2) The thread-local handshakes uses trywait before entering wait:
>>>>>>>>> if (!_semaphore.trywait()) {
>>>>>>>>> - ThreadBlockInVM tbivm(thread);
>>>>>>>>> _semaphore.wait();
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> At the time when SemaphoreSafepointAware was written, we didn't
>>>>>>>>> have trywait on all platforms, now we do. Maybe we should
>>>>>>>>> always do the trywait dance inside
>>>>>>>>> SemaphoreSafepointAware::wait() ?
>>>>>>>>>
>>>>>>>>> inline void SemaphoreSafepointAware::wait() {
>>>>>>>>> if (Thread::current()->is_Java_thread()) {
>>>>>>>>> if (!_semaphore.trywait) {
>>>>>>>>> JavaThread* thread = JavaThread::current();
>>>>>>>>>
>>>>>>>>> // Prepare to block and allow safepoints while blocked
>>>>>>>>> ThreadBlockInVM tbivm(thread);
>>>>>>>>> OSThreadWaitState osts(thread->osthread(), false /* not
>>>>>>>>> in Object.wait() */);
>>>>>>>>>
>>>>>>>>> // Wait for value
>>>>>>>>> _semaphore.wait();
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> StefanK
>>>>>>>>
>>>>>>>
>>>>>
>>
More information about the hotspot-dev
mailing list