On Thu, 24 Sep 2020 06:54:32 GMT, Robbin Ehn <r...@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
