Hi Robbin, We decided to separate thread operation and frame operation. I've posted review request for thread operation. Could you review it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/ We can share HandshakeClosure for GetStackTrace() to GetFrameLocation() as you said. However I wonder why it is not so now. I guess GetStackTrace() would give some overhead (e.g. memory allocation for jvmtiFrameInfo) if we use it for frame location. I thought we should replace VM operation to HandshakeClosure one by one. I will merge these operations as possible in JDK-8248362 if we should do. Thanks, Yasumasa On 2020/06/30 19:23, Robbin Ehn wrote:
Hi Yasumasa, Thanks for your effort doing this. #1 GetFrameLocation GetStackTrace GetCurrentLocation (need to add BCI) Should use exactly the same code since a stack trace with max_count = 1 and start_depth = depth/0 is the frame location and jvmtiFrameInfo contain the correct information (+ add BCI)? Thus GetFrameLocation also would use handshakes and no special handshake path for GetCurrentLocation. So we would have _one_ function to get method and BCI/lineno for depth and max count. Which can easily handle all three cases? (maybe more cases also) Is there nay reason for having a separate path for each of these ??? #2 In this method: JvmtiEnvThreadState::reset_current_location(jvmtiEvent event_type, bool enabled) if (event_type == JVMTI_EVENT_SINGLE_STEP && _thread->has_last_Java_frame()) { We are checking if a running thread have a last Java frame, which means it could have one now, e.g. it could be in another handshake or not woken up from a safepoint yet. So there is no use in checking that. (old code) 313 if (SafepointSynchronize::is_at_safepoint() || 314 ((Thread::current() == _thread) && (_thread == _thread->active_handshaker()))) { #3 You are using a debug only method here "active_handshaker()". #4 This AND is never true: ((Thread::current() == _thread) && (_thread == _thread->active_handshaker()))) You can't be active handshaker for yourself. Thanks, Robbin On 2020-06-24 08: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
