On Thu, 17 Sep 2020 12:07:15 GMT, Robbin Ehn <[email protected]> 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 incrementally with one additional
> commit since the last revision:
>
> Fixed double checks
> Added NSV
> ProcessResult to enum
> Fixed logging
> Moved _active_handshaker to private
Changes look good, thanks for fixing! I added some comments on the changes.
src/hotspot/share/runtime/handshake.cpp line 464:
> 462:
> 463: const char* executioner_name(Thread* current_thread, Thread* handshakee,
> bool current_is_requester) {
> 464: if (current_thread == handshakee) return "self(JavaThread)";
I think we can remove this line since executioner_name() is only called by the
handshaker.
src/hotspot/share/runtime/handshake.cpp line 508:
> 506: assert(op->_target == NULL || _handshakee == op->_target, "Wrong
> thread");
> 507: log_trace(handshake)("Processing handshake " INTPTR_FORMAT " by
> %s", p2i(op),
> 508: executioner_name(current_thread, _handshakee,
> op == match_op));
With the above change we could even avoid factoring the code into
executioner_name() and just do:
log_trace(handshake)("Processing handshake " INTPTR_FORMAT " by %s%s", p2i(op),
op == match_op ? "handshaker" :
"cooperative",
current_thread->is_VM_thread() ? "(VM
Thread)" : "(JavaThread)");
src/hotspot/share/prims/jvmtiEnvBase.cpp line 908:
> 906: #endif
> 907: Thread* current_thread = Thread::current();
> 908: assert(current_thread == java_thread ||
One extra check here.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1164:
> 1162: #ifdef ASSERT
> 1163: Thread *current_thread = Thread::current();
> 1164: assert(current_thread == thr ||
One extra check here.
-------------
PR: https://git.openjdk.java.net/jdk/pull/151
