RFR: 8238761: Asynchronous handshakes [v4]

[David Holmes]

On Mon, 21 Sep 2020 22:57:55 GMT, Coleen Phillimore <coleenp at 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