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
