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

[Yasumasa Suenaga]

Hi Serguei,

Thanks for your comment!
I will fix them after JDK-8248379.

Yasumasa


On 2020/06/26 13:48, serguei.spitsyn at oracle.com wrote:
> Hi Yasumasa,
> 
> I agree with the approach to separate this into different bugs.
> At least, it would be nice to separate the stack trace functions.
> It will help to better focus on each fix and improve review quality.
> 
> I'd wait for new webrevs from you before diving deep with reviewing.
> A couple of quick comments.
> 
> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/src/hotspot/share/prims/jvmtiEnv.cpp.udiff.html
> 
> I also do not like the complexity of the stack trace updateand extra boolean argument in GetStackTraceClosure.
> It feels like it can be simpler.
> I'd suggest to do some renaming as the identifiers you use are not typical in the jvmtiEnv.cpp:
> target_javathread  => java_thread
> actual_frame_count => frame_count
> 
> The GetStackTraceClosureis better to have the same stack_info() function as VM_op:
>     *stack_info_ptr = op.stack_info();
> At least, this part will be unified between VM_op and HandshakeClosure.
> 
> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/src/hotspot/share/prims/jvmtiThreadState.cpp.udiff.html
> 
> - (JavaThread *)Thread::current() == get_thread(),
> - "must be current thread or at safepoint");
> + current_thread == get_thread() ||
> + (current_thread->is_Java_thread() && (current_thread == get_thread()->active_handshaker())),
> + "must be at safepoint or target thread is suspended");
> 
> 
> There is no check that the target thread is suspended.
> You, probably, wanted to say about handshake instead.
> 
> 
> Thanks,
> Serguei
> 
> 
> On 6/23/20 23: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
>