On Wed, 23 Sep 2020 23:45:49 GMT, Serguei Spitsyn <sspit...@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
