RFR: 8238761: Asynchronous handshakes [v3]

[Daniel D.Daugherty]

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

Thumbs up. I don't think I have anything that is in the must fix category.

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

> 689:   // Note:
> 690:   // calling_thread is the thread that requested the list of monitors for java_thread.
> 691:   // java_thread is thread owning the monitors.

s/is thread/is the thread/

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).

typo - s/executint/executing/
grammar - s/e.g./e.g.,/

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

> 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).
> 693:   // And they all maybe different threads.

typo - (in this context) - s/maybe/may be/

src/hotspot/share/prims/jvmtiEnvBase.hpp line 341:

> 339: class JvmtiHandshakeClosure : public HandshakeClosure {
> 340:  protected:
> 341:   jvmtiError _result;

Thanks for pushing the jvmtiError into common code for JVM/TI handshakes.

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

> 106:       _processed,
> 107:       _succeed,
> 108:       _number_states

Why are these indented by 4 spaces instead of 2 spaces?

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

> 68:     : HandshakeOperation(cl, target), _start_time_ns(start_ns) {}
> 69:   virtual ~AsyncHandshakeOperation() { delete _handshake_cl; };
> 70:   jlong start_time()                 { return _start_time_ns; }

Should this be 'const'? Ignore it if it would fan out too much.

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

> 347:     target->handshake_state()->add_operation(op);
> 348:   } else {
> 349:     log_handshake_info(start_time_ns, op->name(), 0, 0, "(thread dead)");

It might be useful to also log the 'target' thread value here so:

    .... (thread=<ptr-value> is dead)"

Might be something like this:

log_handshake_info(start_time_ns, op->name(), 0, 0, "(thread=" INTPTR_FORMAT " is dead)", p2i(target));

Although you'd probably have to form the string in a buffer and then pass it
to the log_handshake_info() call... sigh...

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

> 448:     return false;
> 449:   }
> 450:   // Operations are added without lock and then the poll is armed.

s/without lock/lock free/

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

> 477:   }
> 478:
> 479:   // If we own the mutex at this point and while owning the mutex

grammar - s/owning the mutex/owning the mutex we/

src/hotspot/share/runtime/interfaceSupport.inline.hpp line 157:

> 155:
> 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());

Can you explain why you had to add this?
Did something show up in testing?

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

> 485:   assert(!thread->is_Java_thread() ||
> 486:          ((JavaThread *) thread)->is_handshake_safe_for(Thread::current()) ||
> 487:          !((JavaThread *) thread)->on_thread_list() ||

Should use "thread->as_Java_thread()" instead of the cast here (2 places).

src/hotspot/share/runtime/thread.hpp line 1360:

> 1358:   bool is_handshake_safe_for(Thread* th) const {
> 1359:     return _handshake.active_handshaker() == th ||
> 1360:            this == th;

I _think_ L1359-60 will fit on one line...

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

> 33:   FilterQueueNode* head;
> 34:   FilterQueueNode* insnode = new FilterQueueNode(data);
> 35:   SpinYield yield(SpinYield::default_spin_limit * 10); // Very unlikely with mutiple failed CAS.

Typo - s/mutiple/multiple/

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

> 74:     return (E)NULL;
> 75:   }
> 76:   SpinYield yield(SpinYield::default_spin_limit * 10); // Very unlikely with mutiple failed CAS.

typo - s/mutiple/multiple/

src/hotspot/share/prims/whitebox.cpp line 2032:

> 2030:     void do_thread(Thread* th) {
> 2031:       assert(th->is_Java_thread(), "sanity");
> 2032:       JavaThread* jt = (JavaThread*)th;

Can whitebox.cpp code use the new as_Java_thread() call?

src/hotspot/share/prims/whitebox.cpp line 2033:

> 2031:       assert(th->is_Java_thread(), "sanity");
> 2032:       JavaThread* jt = (JavaThread*)th;
> 2033:       ResourceMark rm;

It also might be interesting to print the "current thread" info here so
that someone looking at the test output knows which thread handled
the handshake (the target or a surrogate).

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

> 43: // a single target/direct handshake or not, by the JavaThread that requested the
> 44: // handshake or the VMThread respectively.
> 45: class HandshakeClosure : public ThreadClosure, public CHeapObj<mtThread> {

Just to be clear. You haven't added support for a handshake that
must only be executed by the target thread yet, right? That's
future work, if I remember correctly...

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

Marked as reviewed by dcubed (Reviewer).

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