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

Tue Jun 30 23:05:31 UTC 2020

Hi David,

>>> 1271     ResourceMark rm;
>>>
>>> IIUC at this point the _calling_thread is the current thread, so we can use:
>>>
>>>      ResourceMark rm(_calling_thread);

If so, we can call make_local() in L1272 without JavaThread (or we can pass current thread to make_local()). Is it right?

```
1271     ResourceMark rm;
1272     _collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread, thread_oop),
1273                            jt, thread_oop);
```

Thanks,

Yasumasa

On 2020/07/01 7:05, David Holmes wrote:
> On 1/07/2020 12:17 am, Yasumasa Suenaga wrote:
>> 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;
>> }
>> ```
> 
> Yes that I what I meant.
> 
> Thanks,
> David
> -----
> 
>>
>> 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