8203341: Add a safepoint-aware Semaphore
Stefan Karlsson
stefan.karlsson at oracle.com
Mon May 21 14:42:34 UTC 2018
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.
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