On Mon, 19 Apr 2021 07:13:57 GMT, Robbin Ehn <[email protected]> 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
