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

[Yasumasa Suenaga]

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