RFR: 8257831: Suspend with handshakes [v10]

[David Holmes]

On Mon, 19 Apr 2021 07:13:57 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> src/hotspot/share/runtime/handshake.hpp line 133:
>> 
>>> 131:     // check the _lock, if held go to slow path.
>>> 132:     // Since the handshakee is unsafe if _lock gets locked after this check
>>> 133:     // we know another thread cannot process any handshakes.
>> 
>> I can't quite understand this comment. I'm not sure what thread is calling this method and when, in relation to what the handshakee may be doing.
>
> The handshakee is in a safe state, e.g. blocked and does:
> Point A: set_thread_state_fence(_thread_blocked_trans);
> ...
> Point B: _lock.is_locked()
> 
> While the processor thread does:
> Point X: _lock.try_lock();
> ...
> Point Z: thread->thread_state();
> 
> If point B is passed with a non-locked lock, point Z is guaranteed to see an unsafe thread and will not start to process ay handshakes.
> 
> Is that make sense, maybe the comment make more sense ? :)

The comment makes more sense now :)

>> src/hotspot/share/runtime/handshake.hpp line 157:
>> 
>>> 155:   Thread* active_handshaker() const { return _active_handshaker; }
>>> 156: 
>>> 157:   // Suspend/resume support
>> 
>> Taking a step back I can't help but think that this is entirely the wrong place to have the suspend/resume API and supporting code. It is only here because we re-use the HandshakeState _lock monitor right? If we introduced a new thread-suspension monitor instead then this code would all reside somewhere else - probably in the JavaThread class.
>
> When going to blocked inside the async handshake we **must** unlock the HandshakeState lock.
> Thus _lock.wait() unlocks and gives us something to notify.
> We could do:
> 
> _lock.unlock();
> {
>   MutexLocker(SomeOtherLock) ml;
>   SomeOtherLock.wait();
> }
> _lock.lock();
> 
> 
> Another alternative is to create a friend class which uses the HandshakeState lock and having the API there instead.

Borrowing the HandshakeState lock does create an artificial coupling here. I'd prefer to see this API in a more natural place with friendship used to access the mechanism as needed. Future cleanup though.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3191