RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Yasumasa Suenaga
suenaga at oss.nttdata.com
Wed Jul 1 01:53:55 UTC 2020
Hi,
I uploaded new webrev. Could review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.02/
>>>>> 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?
This function (JvmtiEnvBase::get_stack_trace()) can be called to get own stack trace. For example, we can call GetStackTrace() for current thread at JVMTI event.
So I changed assert as below:
```
820 assert(current_thread == java_thread ||
821 SafepointSynchronize::is_at_safepoint() ||
822 current_thread == java_thread->active_handshaker(),
823 "call by myself / at safepoint / at handshake");
```
Thanks,
Yasumasa
On 2020/07/01 8:48, David Holmes wrote:
> 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