RFR: 8238761: Asynchronous handshakes [v5]

[Robbin Ehn]

On Thu, 24 Sep 2020 06:54:32 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> 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.

Added comment @dholmes-ora
Added constructor @dholmes-ora
Previous renames caused confusing, renamed some methods and moved those not public to private

Running tests

Good to go?

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

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