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

[Yasumasa Suenaga]

On 2020/07/01 8:54, David Holmes wrote:
> 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.

Yes, this is the case of thread_count == 1.

Yasumasa


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