On Mon, 19 Apr 2021 04:19:28 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Robbin Ehn has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed nits > > src/hotspot/share/runtime/handshake.cpp line 630: > >> 628: // This is the closure that prevents a suspended JavaThread from >> 629: // escaping the suspend request. >> 630: class ThreadSuspensionHandshake : public AsyncHandshakeClosure { > > I still find ThreadSuspensionHandShake versus SuspendThreadHandshake to be > too similarly named and with no obvious way to remember which one is which. > Perhaps this one could be called ThreadSelfSuspensionHandshake instead? Fixed > src/hotspot/share/runtime/handshake.cpp line 636: > >> 634: JavaThread* target = thr->as_Java_thread(); >> 635: target->handshake_state()->do_self_suspend(); >> 636: } > > As this must be the current thread we are dealing with can we rename the > variables to indicate that. Fixed > src/hotspot/share/runtime/handshake.hpp line 128: > >> 126: // must take slow path, process_by_self(), if _lock is held. >> 127: bool should_process() { >> 128: // When doing thread suspension the holder of the _lock > > Potentially this could be any async handshake operation - right? It seems odd > to talk specifically about thread suspension in the core of the handshake > code. Changed > src/hotspot/share/runtime/handshake.hpp line 131: > >> 129: // can add an asynchronous handshake to queue. >> 130: // To make sure it is seen by the handshakee, the handshakee must >> first >> 131: // check the _lock, if held go to slow path. > > ..., _and_ if held ... Fixed > 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 ? :) ------------- PR: https://git.openjdk.java.net/jdk/pull/3191