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

[Yasumasa Suenaga]

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.


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
>