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

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

Hi Yasumasa,

Thank you for separating your initial webrev.
I'll do a full review after you address comments from David and Robbin as I'm stepping on the same ground.

Just a quick comment now.

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

I've already asked in prev. round to make this renaming: target_javathread => java_thread
The identifier java_thread is normally used in the jvmtiEnv.cpp functions.
The target_javathread sounds very unusual.

I do not like much the introduction of the GetSingleStackTraceClosure.
It feels like it can be done in a more elegant way.
From the other hand, it is not that bad. :-)

Thanks,
Serguei


On 6/29/20 17:05, Yasumasa Suenaga wrote:

Hi David, Serguei,

I updated webrev for 8242428. Could you review again?
This change migrate to use direct handshake for GetStackTrace() and GetThreadListStackTraces() (when thread_count == 1).

  http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/

VM_GetThreadListStackTrace (for GetThreadListStackTraces) and VM_GetAllStackTraces (for GetAllStackTraces) have inherited VM_GetMultipleStackTraces VM operation which provides the feature to generate jvmtiStackInfo. I modified  VM_GetMultipleStackTraces to a normal C++ class to share with HandshakeClosure for GetThreadListStackTraces (GetSingleStackTraceClosure).

Also I added new testcases which test GetThreadListStackTraces() with thread_count == 1 and with all threads.

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


Thanks,

Yasumasa


On 2020/06/24 15: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