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

[David Holmes]

On 1/07/2020 9:51 am, Yasumasa Suenaga wrote:
> Hi Serguei,
> 
> On 2020/07/01 8:24, serguei.spitsyn at oracle.com wrote:
>> Hi Yasumasa,
>>
>> Thank you for separating your initial webrev.
>> I'll do a full review after you address comments from David and Robbin 
>> as I'm stepping on the same ground.
>>
>> Just a quick comment now.
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/src/hotspot/share/prims/jvmtiEnv.cpp.udiff.html 
>>
>>
>> I've already asked in prev. round to make this renaming: 
>> target_javathread => java_thread
>> The identifier java_thread is normally used in the jvmtiEnv.cpp 
>> functions.
>> The target_javathread sounds very unusual.
> 
> Sorry, I've fixed it. I will update webrev.
> 
> 
>> I do not like much the introduction ofthe GetSingleStackTraceClosure.
>> It feels like it can be done in a more elegant way.
>>  From the other hand, it is not that bad. :-)
> 
> I thought to use handshake on VMThread (!= direct handshake) for 
> GetThreadListStackTraces() to be simplify implementation.
> However it would be queued as VM op (of course STW would not happen), so 
> I introduced GetSingleStackTraceClosure.

Getting multiple stacktraces must be a stop-the-world operation, so that 
the traces are consistent.

David

> 
> Thanks,
> 
> Yasumasa
> 
> 
>> Thanks,
>> Serguei
>>
>>
>> On 6/29/20 17:05, Yasumasa Suenaga wrote:
>>> Hi David, Serguei,
>>>
>>> I updated webrev for 8242428. Could you review again?
>>> This change migrate to use direct handshake for GetStackTrace() and 
>>> GetThreadListStackTraces() (when thread_count == 1).
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/
>>>
>>> VM_GetThreadListStackTrace (for GetThreadListStackTraces) and 
>>> VM_GetAllStackTraces (for GetAllStackTraces) have inherited 
>>> VM_GetMultipleStackTraces VM operation which provides the feature to 
>>> generate jvmtiStackInfo. I modified  VM_GetMultipleStackTraces to a 
>>> normal C++ class to share with HandshakeClosure for 
>>> GetThreadListStackTraces (GetSingleStackTraceClosure).
>>>
>>> Also I added new testcases which test GetThreadListStackTraces() with 
>>> thread_count == 1 and with all threads.
>>>
>>> This change has been tested in serviceability/jvmti 
>>> serviceability/jdwp vmTestbase/nsk/jvmti vmTestbase/nsk/jdi 
>>> vmTestbase/nsk/jdwp.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/06/24 15: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
>>