On Mon, 21 Sep 2020 22:57:55 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> 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 five additional commits since >> the last revision: >> - 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 >> - Rebase version 1.0 > > Looks mostly good to me! Hi Robbin, I've gone back to refresh myself on the previous discussions and (internal) design walk-throughs to get a better sense of these changes. Really the "asynchronous handshake" aspect is only a small part of this. The fundamental change here is that handshakes are now maintained via a per-thread queue, and those handshake operations can, in the general case, be executed by any of the target thread, the requestor (active_handshaker) thread or the VMThread. Hence the removal of the various "JavaThread::current()" assumptions. Unless constrained otherwise, any handshake operation may be executed by the VMThread so we have to take extra care to ensure the code is written to allow this. I'm a little concerned that our detour into direct-handshakes actually lulled us into a false sense of security knowing that an operation would always execute in a JavaThread, and we have now reverted that and allowed the VMThread back in. I understand why, but the change in direction here caught me by surprise (as I had forgotten the bigger picture). It may not always be obvious that the transitive closure of the code from an operation can be safely executed by a non-JavaThread. Then on top of this generalized queuing mechanism there is a filter which allows some control over which thread may perform a given operation - at the moment the only filter isolates "async" operations which only the target thread can execute. In addition another nuance is that when processing a given thread's handshake operation queue, different threads have different criteria for when to stop processing the queue: - the target thread will drain the queue completely - the VMThread will drain the queue of all "non-async" operations** - the initiator for a "direct" operation will drain the queue up to, and including, the synchronous operation they submitted - the initiator for an "async" operation will not process any operation themselves and will simply add to the queue and then continue on their way (hence the "asynchronous") ** I do have some concerns about latency impact on the VMThread if it is used to execute operations that didn't need to be executed by the VMThread! I remain concerned about the terminology conflation that happens around "async handshakes". There are two aspects that need to be separated: - the behaviour of the thread initiating the handshake operation; and - which thread can execute the handshake operation When a thread initiates a handshake operation and waits until that operation is complete (regardless of which thread performed it, or whether the initiator processed any other operations) that is a synchronous handshake operation. When a thread initiates a handshake operation and does not wait for the operation to complete (it does the target->queue()->add(op); and continues on its way) that is an asynchronous handshake operation. The question of whether the operation must be executed by the target thread is orthogonal to whether the operation was submitted as a synchronous or asynchronous operation. So I have problem when you say that an asynchronous handshake operation is one that must be executed by the target thread, as this is not the right characterisation at all. It is okay to constrain things such that an async operation is always executed by the target, but that is not what makes it an async operation. In the general case there is no reason why an async operation might not be executed by the VMThread, or some other JavaThread performing a synchronous operation on the same target. I will go back through the actual handshake code to see if there are specific things I would like to see changed, but that will have to wait until tomorrow. Thanks, David ------------- PR: https://git.openjdk.java.net/jdk/pull/151
