RFR: 8238761: Asynchronous handshakes [v5]

[Robbin Ehn]

On Wed, 23 Sep 2020 03:04:39 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update after Coleen
>
> src/hotspot/share/runtime/handshake.cpp line 63:
> 
>> 61: };
>> 62:
>> 63: class AsyncHandshakeOperation : public HandshakeOperation {
> 
> This doesn't quite make sense. If you have an AsyncHandshakeOperation as a distinct subclass then it should not be
> possible for is_async() on a HandshakeOperation to return true - but it can because it can be passed an
> AsyncHandshakeClosure when constructed. If you want async and non-async operations to be distinct types then you will
> need to restrict how the base class is constructed, and provide a protected constructor that just takes an
> AsyncHandShakeClosure.

This implementation code not part of the interface.
By casting the AsyncHandShakeClosure to a HandshakeClosure before instantiating the HandshakeOperation you can still
get is_async() to return true. And there are a loads of other user error which can be done like stack allocating
AsyncHandshakeOperation. Protecting against all those kinds of errors requires a lot of more code.

> src/hotspot/share/runtime/handshake.cpp line 44:
> 
>> 42:  protected:
>> 43:   HandshakeClosure*   _handshake_cl;
>> 44:   int32_t             _pending_threads;
> 
> Not new but the meaning of _pending_threads is unclear - please add a descriptive comment.

Fixed

> src/hotspot/share/runtime/handshake.cpp line 195:
> 
>> 193: }
>> 194:
>> 195: static void log_handshake_info(jlong start_time_ns, const char* name, int targets, int requester_executed, const
>> char* extra = NULL) {
> 
> It is not clear what "requester_executed" actually means here - why is this an int? what does it represent?
> Again we have new terminology "requester" - is that the handshakee or ???

Fixed

> src/hotspot/share/runtime/handshake.cpp line 356:
> 
>> 354: }
>> 355:
>> 356: HandshakeState::HandshakeState(JavaThread* thread) :
> 
> s/thread/target/ for clarity

Fixed

> src/hotspot/share/runtime/handshake.cpp line 412:
> 
>> 410:     if (op != NULL) {
>> 411:       assert(op->_target == NULL || op->_target == Thread::current(), "Wrong thread");
>> 412:       assert(_handshakee == Thread::current(), "Wrong thread");
> 
> You already asserted this at line 400.

Fixed

> src/hotspot/share/runtime/thread.hpp line 1358:
> 
>> 1356:   HandshakeState* handshake_state() { return &_handshake; }
>> 1357:
>> 1358:   // A JavaThread can always safely operate on it self and other threads
> 
> s/it self/itself/

Fixed

> src/hotspot/share/runtime/thread.hpp line 1359:
> 
>> 1357:
>> 1358:   // A JavaThread can always safely operate on it self and other threads
>> 1359:   // can do it safely it if they are the active handshaker.
> 
> s/it if/if/

Fixed

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

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