On Thu, 17 Sep 2020 19:51:24 GMT, Robbin Ehn <r...@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
