RFR: 8238761: Asynchronous handshakes

[Patricio Chilano]

Hi Robbin,

Changes look good to me! Some minor comments:

src/hotspot/share/prims/jvmtiThreadState.cpp
222:   assert(current_thread == get_thread() ||
223:          SafepointSynchronize::is_at_safepoint() ||
224: get_thread()->is_handshake_safe_for(current_thread),
225:          "call by myself / at safepoint / at handshake");
Extra current_thread == get_thread() is already handled by 
is_handshake_safe_for().

src/hotspot/share/prims/jvmtiEnvBase.cpp
Same as above.

src/hotspot/share/runtime/handshake.cpp
198:     log_info(handshake)("Handshake \"%s\", Targeted threads: %d, 
Executed by targeted threads: %d, Total completion time: " JLONG_FORMAT 
" ns%s%s",
199:                         name, targets,
200:                         targets - vmt_executed,
In the calls to log_handshake_info() in VM_HandshakeAllThreads and 
Handshake::execute() we are passing as vmt_executed the number of 
handshakes that the driver executed which could be more than the targets 
parameter. So the operation "targets - vmt_executed" to calculate the 
handshakes executed by the targets would no longer be valid. Personally 
I would just leave ProcessResult as an enum and log as before. We still 
have a log_trace() in try_process(), so that already keeps track of 
extra handshakes executed by the handshaker.

src/hotspot/share/runtime/handshake.cpp
387:     NoSafepointVerifier nsv;
388:     process_self_inner();
389:   }
Shouldn't NoSafepointVerifier be placed before the execution of the 
handshake closure so that we also cover the case when the handshake is 
executed by the handshaker? As in:
// Only actually execute the operation for non terminated threads.
if (!thread->is_terminated()) {
     NoSafepointVerifier nsv;
     _handshake_cl->do_thread(thread);
}

src/hotspot/share/runtime/interfaceSupport.inline.hpp
156:     // Threads shouldn't block if they are in the middle of 
printing, but...
157: ttyLocker::break_tty_lock_for_safepoint(os::current_thread_id());
What's the issue of having NoSafepointVerifier inside the handshake?

Thanks!

Patricio

On 9/15/20 4:39 AM, Robbin Ehn 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 the NoSafepointVerifyer.
>
> - Added filtered queue and gtest for it.
>
> Passes multiple t1-8 runs.
> Been through some pre-reviwing.
>
> -------------
>
> Commit messages:
>   - Rebase version 1.0
>
> Changes: https://git.openjdk.java.net/jdk/pull/151/files
>   Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=151&range=00
>    Issue: https://bugs.openjdk.java.net/browse/JDK-8238761
>    Stats: 1047 lines in 24 files changed: 693 ins; 150 del; 204 mod
>    Patch: https://git.openjdk.java.net/jdk/pull/151.diff
>    Fetch: git fetch https://git.openjdk.java.net/jdk pull/151/head:pull/151
>
> PR: https://git.openjdk.java.net/jdk/pull/151