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

[Robbin Ehn]

Hi Yasumasa,

On 2020-06-30 14:35, Yasumasa Suenaga wrote:
> Hi Robbin,
> 
> We decided to separate thread operation and frame operation.
> I've posted review request for thread operation. Could you review it?

Yes I know but I'm soon off for vacation and you have not sent them all
out and due to the nature of my comments replied here.

> 
>    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.

In case of GetFrameLocation we only need one and it's only a dozen bytes 
big I wouldn't count that as an overhead in this case.
If you are really paranoid you can stack allocated it and pass it in.

> 
> 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.

If you make the first one generic enough or evolve it you can convert VM
op to your new function instead, thus doing one by one.

So in http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/ I 
really think:

1575 JvmtiEnv::GetThreadListStackTraces(jint thread_count, const 
jthread* thread_list, jint max_frame_count, jvmtiStackInfo** 
stack_info_ptr) {
.....
1578   if (thread_count == 1) {
===>     // This case should be the same as JvmtiEnv::GetStackTrace
1592   } else {

I do not see any issues, so it seem reasonable, thanks.

Thanks, Robbin

> 
> 
> 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