On Mon, 28 Sep 2020 09:01:22 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 with a new target base due to a merge > or a rebase. The incremental webrev > excludes the unrelated changes brought in by the merge/rebase. The pull > request contains 11 additional commits since > the last revision: > - More fixes from David > - Merge branch 'master' into 8238761-asynchrounous-handshakes > - Add constructor and comment. Previous renames caused confusing, improved > names once more and moved non-public to private > - Fixed trailing whitespace > - Update after David > - Update after Coleen > - Update after Dan and David > - Merge branch 'master' into 8238761-asynchrounous-handshakes > - Removed double check, fix comment, removed not needed function, updated > logs > - Fixed double checks > Added NSV > ProcessResult to enum > Fixed logging > Moved _active_handshaker to private > - ... and 1 more: > https://git.openjdk.java.net/jdk/compare/153ca635...3a95750e Hi Robbin, Thanks for the updates and the slack chat to clarify my misunderstanding of the queuing mechanism. I agree that the logging statements are somewhat confusing as they were written when the processing logic was much simpler, but I understand now the count of emitted executed operations. This all looks good to me now. Thanks, David ------------- Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/151
