RFR: 8238761: Asynchronous handshakes [v5]

[Robbin Ehn]

On Wed, 23 Sep 2020 23:45:49 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> No significant comments. All my concerns relate to naming and terminology, where I think there is scope for quite a bit
>> of tidying up. Thanks.
>
> Hi Robbin,
> I've looked more at the Serviceability files.
> The fix looks good in general. Nice refactoring and simplification with the JvmtiHandshakeClosure.
> It is a little unexpected that the do_thread can be executed by non-JavaThread's.
> Not sure, what kinds of inconvenience it can cause.
> Also, adding the _completed field is somewhat inconsistent.
> I'd expect it to be present or not present for almost all JVMTI handshake closures.
> I hope, you can comment on why it was added in these particular cases.

Hi Serguei,

> Hi Robbin,
> I've looked more at the Serviceability files.
> The fix looks good in general. Nice refactoring and simplification with the JvmtiHandshakeClosure.

Good.

> It is a little unexpected that the do_thread can be executed by non-JavaThread's.
> Not sure, what kinds of inconvenience it can cause.

Reading the code I did not find any issues.
Any return values must already be allocated correctly since the executor thread do not wait around.
Thus targeted thread must be the owner of these.
Also extensive testing find no issues.
But as I said to David, we can easily change back to the previous behavior if needed by using the filter mechanism.

> Also, adding the _completed field is somewhat inconsistent.
> I'd expect it to be present or not present for almost all JVMTI handshake closures.
> I hope, you can comment on why it was added in these particular cases.

Two of the handshakes have guarantee checks:
https://github.com/openjdk/jdk/blob/94daf2c7566d882c46cb87b8c28017812bf722ef/src/hotspot/share/prims/jvmtiEnvThreadState.cpp#L326
https://github.com/openjdk/jdk/blob/94daf2c7566d882c46cb87b8c28017812bf722ef/src/hotspot/share/prims/jvmtiEventController.cpp#L345

This is the only placed it is used/needed.
In my previous version I just removed those guarantee because they needed extra code and was suspicious to me.
But it was requested to add them back.

I had quick look now, and from what I can tell it is not guaranteed that we do execute those handshake.
We do not run handshakes on exiting threads (is_terminated()), we set is exiting here:
https://github.com/openjdk/jdk/blob/94daf2c7566d882c46cb87b8c28017812bf722ef/src/hotspot/share/runtime/thread.cpp#L2123
But we remove the JVM TI Thread state way later, here:
https://github.com/openjdk/jdk/blob/94daf2c7566d882c46cb87b8c28017812bf722ef/src/hotspot/share/runtime/thread.cpp#L2207

For example the method ensure_join() which is between set_terminated and removing the JVM TI state can
safepoint/handshake.
https://github.com/openjdk/jdk/blob/94daf2c7566d882c46cb87b8c28017812bf722ef/src/hotspot/share/runtime/thread.cpp#L2159

That would trigger the guarantee.

So I believe we should not have those two guarantees and thus the _completed can be removed once again.
I think that should be handled in a separate issue, leaving this as is for now.

-------------

PR: https://git.openjdk.java.net/jdk/pull/151