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

[David Holmes]

Hi Robbin,

On 30/06/2020 11:03 pm, Robbin Ehn wrote:
> 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 {

It effectively is, but it can't be exactly the same because the inputs 
and outputs are different.

There is an opportunity for more refactoring and streamlining if we look 
at the flow of arguments through the whole call-chain (as per my other 
email) but that's a bit beyond the scope of this initial conversion I think.

Cheers,
David
-----

> 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