8203341: Add a safepoint-aware Semaphore

[Per Liden]

Looks good!

/Per

On 05/18/2018 02: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
>>>>>
>>>>
>>