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

[Robbin Ehn]

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