8203341: Add a safepoint-aware Semaphore

[Stefan Karlsson]

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
>