On Wed, 23 Sep 2020 03:04:39 GMT, David Holmes <dhol...@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
