RFR: 8238761: Asynchronous handshakes [v4]

[Coleen Phillimore]

On Mon, 21 Sep 2020 12:19:09 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> This patch implements asynchronous handshake, which changes how handshakes works by default. Asynchronous handshakes
>> are target only executed, which they may never be executed. (target may block on socket for the rest of VM lifetime)
>> Since we have several use-cases for them we can have many handshake pending. (should be very rare) To be able handle an
>> arbitrary amount of handshakes this patch adds a per JavaThread queue and heap allocated HandshakeOperations. It's a
>> singly linked list where you push/insert to the end and pop/get from the front. Inserts are done via CAS on first
>> pointer, no lock needed. Pops are done while holding the per handshake state lock, and when working on the first
>> pointer also CAS.  The thread grabbing the handshake state lock for a JavaThread will pop and execute all handshake
>> operations matching the filter. The JavaThread itself uses no filter and any other thread uses the filter of everything
>> except asynchronous handshakes. In this initial change-set there is no need to do any other filtering. If needed
>> filtering can easily be exposed as a virtual method on the HandshakeClosure, but note that filtering causes handshake
>> operation to be done out-order. Since the filter determins who execute the operation and not the invoked method, there
>> is now only one method to call when handshaking one thread.  Some comments about the changes:
>> - HandshakeClosure uses ThreadClosure, since it neat to use the same closure for both alla JavThreads do and Handshake
>>   all threads. With heap allocating it cannot extends StackObj. I tested several ways to fix this, but those very much
>>   worse then this.
>> 
>> - I added a is_handshake_safe_for for checking if it's current thread is operating on itself or the handshaker of that
>>   thread.
>> 
>> - Simplified JVM TI with a JvmtiHandshakeClosure and also made them not needing a JavaThread when executing as a
>>   handshaker on a JavaThread, e.g. VM Thread can execute the handshake operation.
>> 
>> - Added WB testing method.
>> 
>> - Removed VM_HandshakeOneThread, the VM thread uses the same call path as direct handshakes did.
>> 
>> - Changed the handshake semaphores to mutex to be able to handle deadlocks with lock ranking.
>> 
>> - VM_HandshakeAllThreadsis still a VM operation, since we do support half of the threads being handshaked before a
>>   safepoint and half of them after, in many handshake all operations.
>> 
>> - ThreadInVMForHandshake do not need to do a fenced transistion since this is always a transistion from unsafe to unsafe.
>> 
>> - Added NoSafepointVerifyer, we are thinking about supporting safepoints inside handshake, but it's not needed at the
>>   moment. To make sure that gets well tested if added the NoSafepointVerifyer will raise eyebrows.
>> 
>> - Added ttyLocker::break_tty_lock_for_safepoint(os::current_thread_id()); due to lock rank.
>> 
>> - Added filtered queue and gtest for it.
>> 
>> Passes multiple t1-8 runs.
>> Been through some pre-reviwing.
>
> Robbin Ehn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev
> excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since
> the last revision:
>  - Update after Dan and David
>  - Merge branch 'master' into 8238761-asynchrounous-handshakes
>  - Removed double check, fix comment, removed not needed function, updated logs
>  - Fixed double checks
>    Added NSV
>    ProcessResult to enum
>    Fixed logging
>    Moved _active_handshaker to private
>  - Rebase version 1.0

Looks mostly good to me!

src/hotspot/share/runtime/handshake.hpp line 55:

> 53: };
> 54:
> 55: class AsynchHandshakeClosure : public HandshakeClosure {

Can you make this minor change?  Asynch to english speakers looks like a-cinch and if left as Async is a-sink. Can you
remove the 'h's ?  I see David above left off the extra h, which is what one expects this to be named.

src/hotspot/share/runtime/handshake.hpp line 78:

> 76:   FilterQueue<HandshakeOperation*> _queue;
> 77:   Mutex _lock;
> 78:   Thread* _active_handshaker;

Nit: can you line up the data member names for lock and _active_handshaker ?

src/hotspot/share/runtime/handshake.cpp line 394:

> 392:   {
> 393:     NoSafepointVerifier nsv;
> 394:     process_self_inner();

Can you remove process_self_inner and just inline it here since this is it's only caller and both are short functions?
If you don't want to, that's fine.  I found myself searching for any other callers of this, that's all.

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

Changes requested by coleenp (Reviewer).

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