RFR: 8238761: Asynchronous handshakes [v3]

[David Holmes]

On Thu, 17 Sep 2020 19:51:24 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:
> 
>   Removed double check, fix comment, removed not needed function, updated logs

Hi Robbin,

There is still a lack of motivation for this feature in the JBS issue. What kind of handshakes need to be asynchronous?
Any async operation implies that the requester doesn't care about when or even if, the operation gets executed - they
are by definition fire-and-forget actions. So what are the usecases being envisaged here?

Many of the changes included here seem unrelated to, and not reliant on, async handshakes, and could be factored out to
simplify the review and allow focus on the actual async handshake part e.g. the JVM TI cleanups seem they could be
mostly standalone.

Specific comments below. A general concern I have is where the current thread is no longer guaranteed to be a
JavaThread (which is a step in the wrong direction in relation to some of the cleanups I have planned!) and I can't see
why this would be changing.

Thanks.

src/hotspot/share/prims/jvmtiEnvThreadState.cpp line 320:

> 318:         op.do_thread(_thread);
> 319:       } else {
> 320:         Handshake::execute(&op, _thread);

We should still have the guarantee check that the target was alive.

src/hotspot/share/prims/jvmtiEventController.cpp line 339:

> 337:     hs.do_thread(target);
> 338:   } else {
> 339:     Handshake::execute(&hs, target);

We should still have the guarantee check that the target is still alive.

src/hotspot/share/prims/jvmtiEnvBase.cpp line 692:

> 690:   // calling_thread is the thread that requested the list of monitors for java_thread.
> 691:   // java_thread is thread owning the monitors.
> 692:   // current_thread is thread executint this code, can be a non-JavaThread (e.g. VM Thread).

Where is the use of the VMThread introduced as I cannot see it? The code was changed to use direct handshakes so that
we know that either the caller or the target (both JavaThreads) must be executing it. I don't see any VMOperation added
to call this code.

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

> 228:     log_trace(handshake)("Threads signaled, begin processing blocked threads by VMThread");
> 229:     HandshakeSpinYield hsy(start_time_ns);
> 230:     int executed_by_driver = 0;

driver?? Isn't this still the VMThread?

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

> 311:   }
> 312:
> 313:   int executed_by_driver = 0;

Again why driver?? Isn't it either the current thread or the target that will execute the op?

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

> 451:   // If all handshake operations for the handshakee are finished and someone
> 452:   // just adds an operation we may see it here. But if the handshakee is not
> 453:   // armed yet it is not safe to procced.

s/procced/proceed/

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

> 42: // by the target JavaThread itself or, depending on whether the operation is
> 43: // a single target/direct handshake or not, by the JavaThread that requested the
> 44: // handshake or the VMThread respectively.

This comment now indicates that all single target handshakes are executed as direct-handshakes and never by the
VMThread - is that correct?

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

> 105:       _claim_failed,
> 106:       _processed,
> 107:       _succeed,

grammatically should be _succeeded

src/hotspot/share/runtime/thread.cpp line 481:

> 479: #ifdef ASSERT
> 480: // A JavaThread is considered "dangling" if it is not the current
> 481: // thread, his not handshaking with current thread, as been added the Threads

s/his/is/ ?
s/as been added the/has been added to the/

But rather than describe all the conditions, few of which are actually visible in the assertion below, why not just
rephrase in terms of the conditions checked i.e. // A JavaThread is considered dangling if it not handshake-safe with
respect to the current thread, or // it is not on a ThreadsList.
The is_handshake_safe_for method should describe all the conditions that make a target handshake safe.

src/hotspot/share/runtime/thread.cpp line 851:

> 849: bool
> 850: JavaThread::is_thread_fully_suspended(bool wait_for_suspend, uint32_t *bits) {
> 851:   if (this != Thread::current()) {

Why/how is a non-JavaThread calling this?

src/hotspot/share/runtime/thread.cpp line 2441:

> 2439:   assert(Thread::current()->is_VM_thread() ||
> 2440:          is_handshake_safe_for(Thread::current()),
> 2441:          "should be in the vm thread, self or handshakee");

This seems too general. This should either be a VMoperation or a direct handshake, but not both.

src/hotspot/share/utilities/filterQueue.hpp line 57:

> 55:
> 56:   // MT-safe
> 57:   // Since pops and adds are allowed while we add, we do not know if _first is same even if it's the same address.

This comment seems out of context on the declaration of add, as it is describing a detail of the implementation - to
what are we comparing _first to be the same ?? If you want to just document the MT properties abstractedly then
describing this a lock-free would suffice. Though if pop requires locking then it's implementation is also constrained
to work with the lock-free version of add. Overall it is unclear how documenting "external serialization needed"
actually helps the user of this API. ??

src/hotspot/share/utilities/filterQueue.inline.hpp line 42:

> 40:       break;
> 41:     }
> 42:     yield.wait();

Was the need for spinwaits identified through benchmarking? Do you really expect this to be hot?

src/hotspot/share/utilities/filterQueue.inline.hpp line 63:

> 61: }
> 62:
> 63: // MT-Unsafe, external serialization needed.

So IIUC this queue supports multiple concurrent add()s, but has to be restricted to a single pop() at a time (although
that is allowed to execute concurrently with the add()s) - correct?

test/hotspot/jtreg/runtime/handshake/AsyncHandshakeWalkStackTest.java line 30:

> 28:  * @build AsyncHandshakeWalkStackTest
> 29:  * @run driver ClassFileInstaller sun.hotspot.WhiteBox
> 30:  *                              sun.hotspot.WhiteBox$WhiteBoxPermission

This is not needed as ClassFileInstaller already handles WhiteBox's nested classes.

test/hotspot/jtreg/runtime/handshake/MixedHandshakeWalkStackTest.java line 30:

> 28:  * @build MixedHandshakeWalkStackTest
> 29:  * @run driver ClassFileInstaller sun.hotspot.WhiteBox
> 30:  *                              sun.hotspot.WhiteBox$WhiteBoxPermission

Not needed - see earlier comment.

test/hotspot/jtreg/runtime/handshake/MixedHandshakeWalkStackTest.java line 38:

> 36:
> 37: public class MixedHandshakeWalkStackTest {
> 38:     public static Thread tthreads[];

why tthreads? It looks like a typo :)

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

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