8203341: Add a safepoint-aware Semaphore

[David Holmes]

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

   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?

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

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