On Thu, 24 Sep 2020 08:18:12 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:
>
> Add constructor and comment. Previous renames caused confusing, improved
> names once more and moved non-public to private
Still some naming issues to resolve and an editing pass on various new comments.
Thanks,
David
src/hotspot/share/runtime/handshake.cpp line 51:
> 49: private:
> 50: // Must use AsyncHandshakeOperation when using AsyncHandshakeClosure.
> 51: HandshakeOperation(AsyncHandshakeClosure* cl, JavaThread* target, jlong
> start_ns) {};
I'm really not understanding the way you have implemented the guard I
requested. How does declaring the private
constructor prevent a call to HandShakeOperation(someAsync, target) ?
src/hotspot/share/runtime/handshake.cpp line 202:
> 200: }
> 201:
> 202: static void log_handshake_info(jlong start_time_ns, const char* name,
> int targets, int non_self_executed, const
> char* extra = NULL) {
The name "non_self_executed" is much clearer - thanks. But this now highlights
that the log message itself doesn't make
complete sense as it refers to "requesting thread" when it could be a range of
possible threads not just a single
requesting thread.
src/hotspot/share/runtime/handshake.cpp line 239:
> 237: // Keeps count on how many of own emitted handshakes
> 238: // this thread execute.
> 239: int emitted_handshakes_executed = 0;
I suggest:
// Keeps count of how many handshakes were actually executed
// by this thread.
int handshakes_executed = 0;
src/hotspot/share/runtime/handshake.cpp line 326:
> 324: // Keeps count on how many of own emitted handshakes
> 325: // this thread execute.
> 326: int emitted_handshakes_executed = 0;
See previous comment.
src/hotspot/share/runtime/handshake.hpp line 79:
> 77: // Provides mutual exclusion to this state and queue.
> 78: Mutex _lock;
> 79: // Set to the thread executing the handshake operation during the
> execution.
s/the execution/its execution/
Or just delete "during the execution"
src/hotspot/share/runtime/handshake.hpp line 87:
> 85: void process_self_inner();
> 86:
> 87: bool have_non_self_executable_operation();
API naming consistency is still a problem here. We have:
- pop_for_self()
- pop()
but
- has_operation() // for use by self/handshakee
- have_non_self_executable_operation()
why not:
- has_operation_for_self()
- has_operation()
?
src/hotspot/share/runtime/handshake.hpp line 100:
> 98: }
> 99:
> 100: // Both _queue and _lock must be check. If a thread have seen this
> _handshakee
s/check/checked/
s/have/has/
src/hotspot/share/runtime/handshake.hpp line 103:
> 101: // as safe it will execute all possible handshake operations in a loop
> while
> 102: // holding _lock. We use lock free addition to the queue, which means
> it is
> 103: // possible to the queue to be seen as empty by _handshakee but as
> non-empty
s/to/for/
src/hotspot/share/runtime/handshake.hpp line 105:
> 103: // possible to the queue to be seen as empty by _handshakee but as
> non-empty
> 104: // by the thread executing in the loop. To avoid the _handshakee
> eliding
> 105: // stopping while handshake operations are being executed, the
> _handshakee
s/eliding stopping/continuing/ ?
src/hotspot/share/runtime/handshake.hpp line 106:
> 104: // by the thread executing in the loop. To avoid the _handshakee
> eliding
> 105: // stopping while handshake operations are being executed, the
> _handshakee
> 106: // must take slow if _lock is held and make sure the queue is empty
> otherwise
Unclear what "slow" refers to - "take the slow path"? But what path is that?
src/hotspot/share/runtime/handshake.hpp line 107:
> 105: // stopping while handshake operations are being executed, the
> _handshakee
> 106: // must take slow if _lock is held and make sure the queue is empty
> otherwise
> 107: // try process it.
s/try/try to/
but I'm not sure how this follows the "otherwise" in this sentence.
src/hotspot/share/utilities/filterQueue.hpp line 32:
> 30:
> 31: // The FilterQueue is FIFO with the ability to skip over queued items.
> 32: // The skipping is controlled by using a filter when poping.
s/poping/popping/
src/hotspot/share/utilities/filterQueue.hpp line 33:
> 31: // The FilterQueue is FIFO with the ability to skip over queued items.
> 32: // The skipping is controlled by using a filter when poping.
> 33: // It also supports lock free pushes, while poping (including contain())
s/poping/popping/
s/contain()/contains()/
src/hotspot/share/utilities/filterQueue.hpp line 63:
> 61:
> 62: // Applies the match_func to the items in the queue until match_func
> returns
> 63: // true and then return false, or there is no more items and then
> returns
Surely "true and then returns true"?
s/there is/there are/
src/hotspot/share/utilities/filterQueue.hpp line 64:
> 62: // Applies the match_func to the items in the queue until match_func
> returns
> 63: // true and then return false, or there is no more items and then
> returns
> 64: // false. Any pushed item while executing may or may not have match_func
s/pushed item/item pushed/
I know what you are trying to say about concurrent pushes but it sounds too
non-deterministic - any concurrent push
that happens before contains() reaches the end of the queue will have the
match_func applied. So in that sense "while
executing" only applies to pushes that will be seen; any push not seen had to
have happened after execution was
complete.
src/hotspot/share/utilities/filterQueue.hpp line 66:
> 64: // false. Any pushed item while executing may or may not have match_func
> 65: // applied. The method is not re-entrant and must be executed mutually
> 66: // exclusive other contains and pops calls.
// exclusive to other contains() and pop() calls.
src/hotspot/share/utilities/filterQueue.hpp line 77:
> 75:
> 76: // Applies the match_func to all items in the queue returns the item
> which
> 77: // match_func return true for and was inserted first. Any pushed item
> while
// Applies the match_func to each item in the queue, in order of insertion, and
// returns the first item for which match_func returns true. Returns false if
there are
// no matches or the queue is empty.
src/hotspot/share/utilities/filterQueue.hpp line 80:
> 78: // executing may or may not have be popped, if popped it was the first
> 79: // inserted match. The method is not re-entrant and must be executed
> mutual
> 80: // exclusive with other contains and pops calls.
See comments on contains() to ensure pop() and contains() use consistent
terminology. Thanks.
src/hotspot/share/runtime/handshake.cpp line 389:
> 387: };
> 388:
> 389: static bool non_self_queue_filter(HandshakeOperation* op) {
The name "non_self_queue_filter" is really awkward - sorry. But I think we're
going to have to revisit the way
filtering is named and done once we try to generalise it anyway, so I'll let
this pass.
-------------
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/151
