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

Tue Jun 30 14:17:20 UTC 2020

Hi David,

Thank you for reviewing! I will update new webrev tomorrow.

> 466 class MultipleStackTracesCollector : public StackObj {
> 
>   498 class VM_GetAllStackTraces : public VM_Operation {
>   499 private:
>   500   JavaThread *_calling_thread;
>   501   jint _final_thread_count;
>   502   MultipleStackTracesCollector _collector;
> 
> You can't have a StackObj as a member of another class like that as it may not be on the stack. I think MultipleStackTracesCollector should not extend any allocation class, and should always be embedded directly in another class.

I'm not sure what does mean "embedded".
Is it ok as below?

```
class MultipleStackTracesCollector {
    :
}

class GetAllStackTraces : public VM_Operation {
   private:
     MultipleStackTracesCollector _collector;
}
```

Thanks,

Yasumasa

On 2020/06/30 22:22, David Holmes wrote:
> Hi Yasumasa,
> 
> On 30/06/2020 10:05 am, 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/
> 
> This looks really good now! I only have a few nits below. There is one thing I don't like about it but it requires a change to the main Handshake logic to address - in JvmtiEnv::GetThreadListStackTraces you have to create a ThreadsListHandle to convert the jthread to a JavaThread, but then the Handshake::execute_direct creates another ThreadsListHandle internally. That's a waste. I will discuss with Robbin and file a RFE to have an overload of execute_direct that takes an existing TLH. Actually it's worse than that because we have another TLH in use at the entry point for the JVMTI functions, so I think there may be some scope for simplifying the use of TLH instances - future RFE.
> 
> ---
> 
> src/hotspot/share/prims/jvmtiEnvBase.hpp
> 
>   451   GetStackTraceClosure(JvmtiEnv *env, jint start_depth, jint max_count,
>   452                        jvmtiFrameInfo* frame_buffer, jint* count_ptr)
>   453     : HandshakeClosure("GetStackTrace"),
>   454       _env(env), _start_depth(start_depth), _max_count(max_count),
>   455       _frame_buffer(frame_buffer), _count_ptr(count_ptr),
>   456       _result(JVMTI_ERROR_THREAD_NOT_ALIVE) {
> 
> Nit: can you do one initializer per line please.
> 
> This looks wrong:
> 
> 466 class MultipleStackTracesCollector : public StackObj {
> 
>   498 class VM_GetAllStackTraces : public VM_Operation {
>   499 private:
>   500   JavaThread *_calling_thread;
>   501   jint _final_thread_count;
>   502   MultipleStackTracesCollector _collector;
> 
> You can't have a StackObj as a member of another class like that as it may not be on the stack. I think MultipleStackTracesCollector should not extend any allocation class, and should always be embedded directly in another class.
> 
> 481   MultipleStackTracesCollector(JvmtiEnv *env, jint max_frame_count) {
>   482     _env = env;
>   483     _max_frame_count = max_frame_count;
>   484     _frame_count_total = 0;
>   485     _head = NULL;
>   486     _stack_info = NULL;
>   487     _result = JVMTI_ERROR_NONE;
>   488   }
> 
> As you are touching this can you change it to use an initializer list as you did for the HandshakeClosure, and please keep one item per line.
> 
> ---
> 
> src/hotspot/share/prims/jvmtiEnvBase.cpp
> 
>   820   assert(SafepointSynchronize::is_at_safepoint() ||
>   821          java_thread->is_thread_fully_suspended(false, &debug_bits) ||
>   822          current_thread == java_thread->active_handshaker(),
>   823          "at safepoint / handshake or target thread is suspended");
> 
> I don't think the suspension check is necessary, as even if the target is suspended we must still be at a safepoint or in a handshake with it. Makes me wonder if we used to allow a racy stacktrace operation on a suspended thread, assuming it would remain suspended?
> 
> 1268   oop thread_oop = jt->threadObj();
> 1269
> 1270   if (!jt->is_exiting() && (jt->threadObj() != NULL)) {
> 
> You can use thread_oop in line 1270.
> 
> 1272 _collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread, thread_oop),
> 1273                            jt, thread_oop);
> 
> It is frustrating that this entire call chain started with a jthread reference, which we converted to a JavaThread, only to eventually need to convert it back to a jthread! I think there is some scope for simplification here but not as part of this change.
> 
> 1271     ResourceMark rm;
> 
> IIUC at this point the _calling_thread is the current thread, so we can use:
> 
>      ResourceMark rm(_calling_thread);
> 
> ---
> 
> Please add @bug lines to the tests.
> 
> I'm still pondering the test logic but wanted to send this now.
> 
> Thanks,
> David
> -----
>> 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