RFR: 8238761: Asynchronous handshakes [v5]
Robbin Ehn
rehn at openjdk.java.net
Wed Sep 23 10:11:40 UTC 2020
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
More information about the serviceability-dev
mailing list