On Tue, 22 Sep 2020 07:00:06 GMT, Robbin Ehn <r...@openjdk.org> wrote:
>> 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 > > Hi David, you are correct and you did a fine job summarizing this, thanks! > >> 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. > > Great thanks! > >> >> Thanks, >> David > > # @coleenp I think you placed your comment: "The "driver" concept is odd. Should it really be caller? Like the thread that called VMHandshake?" On the commit or somewhere else, not a review comment, I can't reply. Anyhow I can reply to it here: The answer is caller makes me think of requester, but we don't know if it's the requester executing this. Therefore I referred to the thread executing handshakes on the target/handshakee as driver. ------------- PR: https://git.openjdk.java.net/jdk/pull/151
