RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
David Holmes
david.holmes at oracle.com
Tue Jun 30 23:48:48 UTC 2020
Hi Yasumasa,
On 1/07/2020 9:05 am, Yasumasa Suenaga wrote:
> 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);
> ```
Sorry I got confused, _calling_thread may not be the current thread as
we could be executing the handshake in the target thread itself. So the
ResourceMark is correct as-is (implicitly for current thread).
The argument to fill_frames will be used in the jvmtiStackInfo and
passed back to the _calling_thread, so it must be created via
make_local(_calling_thread, ...) as you presently have.
Thanks,
David
> 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
More information about the serviceability-dev
mailing list