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

Tue Jun 30 22:05:23 UTC 2020

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