8203341: Add a safepoint-aware Semaphore
David Holmes
david.holmes at oracle.com
Tue May 22 01:19:49 UTC 2018
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