Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake

[serguei.spit...@oracle.com]

Hi Yasumasa,

I agree with the approach to separate this into different bugs.
At least, it would be nice to separate the stack trace functions.
It will help to better focus on each fix and improve review quality.

I'd wait for new webrevs from you before diving deep with reviewing.
A couple of quick comments.

http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/src/hotspot/share/prims/jvmtiEnv.cpp.udiff.html

I also do not like the complexity of the stack trace update and extra boolean argument in GetStackTraceClosure.
It feels like it can be simpler.
I'd suggest to do some renaming as the identifiers you use are not typical in the jvmtiEnv.cpp:
 target_javathread  => java_thread
 actual_frame_count => frame_count

The GetStackTraceClosure is better to have the same stack_info() function as VM_op:
   *stack_info_ptr = op.stack_info();
At least, this part will be unified between VM_op and HandshakeClosure.

http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/src/hotspot/share/prims/jvmtiThreadState.cpp.udiff.html

-    (JavaThread *)Thread::current() == get_thread(),
-    "must be current thread or at safepoint");
+    current_thread == get_thread() ||
+    (current_thread->is_Java_thread() && (current_thread == get_thread()->active_handshaker())),
+    "must be at safepoint or target thread is suspended");

There is no check that the target thread is suspended.
You, probably, wanted to say about handshake instead.


Thanks,
Serguei


On 6/23/20 23:50, Yasumasa Suenaga wrote:

Hi all,

Please review this change:

  JBS: https://bugs.openjdk.java.net/browse/JDK-8242428
  webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/

This change replace following VM operations to direct handshake.

 - VM_GetFrameCount (GetFrameCount())
 - VM_GetFrameLocation (GetFrameLocation())
 - VM_GetThreadListStackTraces (GetThreadListStackTrace())
 - VM_GetCurrentLocation

GetThreadListStackTrace() uses direct handshake if thread count == 1. In other case (thread count > 1), it would be performed as VM operation (VM_GetThreadListStackTraces).
Caller of VM_GetCurrentLocation (JvmtiEnvThreadState::reset_current_location()) might be called at safepoint. So I added safepoint check in its caller.

This change has been tested in serviceability/jvmti serviceability/jdwp vmTestbase/nsk/jvmti vmTestbase/nsk/jdi vmTestbase/ns
k/jdwp.

Also I tested it on submit repo, then it has execution error (mach5-one-ysuenaga-JDK-8242428-20200624-0054-12034717) due to dependency error. So I think it does not occur by this change.


Thanks,

Yasumasa