RFR: 8238761: Asynchronous handshakes [v2]

[David Holmes]

Correction ...

On 21/09/2020 4:16 pm, David Holmes wrote:
> On Thu, 17 Sep 2020 12:07:15 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 incrementally with one additional commit since the last revision:
>>
>>    Fixed double checks
>>    Added NSV
>>    ProcessResult to enum
>>    Fixed logging
>>    Moved _active_handshaker to private
> 
> src/hotspot/share/runtime/handshake.cpp line 336:
> 
>> 334:   // and thus prevents reading stale data modified in the handshake closure
>> 335:   // by the Handshakee.
>> 336:   OrderAccess::acquire();
> 
> How/why is this deleted? Surely there are still single-thread VMops that use a handshake??

That comment was placed against the old line 336 which was the deletion 
of this method:

bool Handshake::execute(HandshakeClosure* thread_cl, JavaThread* target) {

(I'll file a skara/git bug).

David
-----

> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 136:
> 
>> 134:     assert(_thread->thread_state() == _thread_in_vm, "should only call when leaving VM after handshake");
>> 135:
>> 136:     _thread->set_thread_state(_original_state);
> 
> Can you clarify why this is no longer needed? What states can we be returning to?
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/151
>