8203341: Add a safepoint-aware Semaphore

[Erik Österlund]

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
>>>
>>