RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Yasumasa Suenaga
suenaga at oss.nttdata.com
Thu Jul 2 09:19:55 UTC 2020
Hi David,
I upload new webrev. Could you review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.03/
On 2020/07/02 15:05, David Holmes wrote:
> Hi Yasumasa,
>
> On 1/07/2020 11:53 am, Yasumasa Suenaga wrote:
>> Hi,
>>
>> I uploaded new webrev. Could review again?
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.02/
>
> Updates look fine - thanks.
>
> One minor nit:
>
> 1274 _collector.allocate_and_fill_stacks(1);
> 1275 _collector.set_result(JVMTI_ERROR_NONE);
>
> In the other places where you use _collector you rely on result being initialized to JVMTI_ERROR_NONE, rather than setting it directly after allocate_and_fill_stacks().
Fixed.
>>>>>>> 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");
>> ```
>
> Yep good catch. I hope current tests caught that.
They would be tested in vmTestbase/nsk/jvmti/GetStackTrace/getstacktr001/ (own call stacks), and getstacktr003 (call stacks in other thread).
> Speaking of tests ...
>
> In the native code I think you need to check the success of all JNI methods that can throw exceptions - otherwise I believe the tests may trigger warnings if -Xcheck:jni is used with them. See for example:
>
> test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.cpp
I updated testcases to check JNI and JVMTI function calls.
> In the Java code the target thread:
>
> 45 public void run() {
> 46 try {
> 47 synchronized (lock) {
> 48 lock.wait();
> 49 System.out.println("OK");
> 50 }
>
> is potentially susceptible to spurious wakeups. Using a CountDownLatch would be robust.
Fixed.
Thanks,
Yasumasa
> Thanks,
> David
> -----
>
>>
>> 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