RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Jul 3 00:56:39 UTC 2020
On 7/2/20 7:14 PM, Yasumasa Suenaga wrote:
> Hi Dan,
>
> Thanks for your comment!
>
> On 2020/07/03 7:16, Daniel D. Daugherty wrote:
>> On 7/2/20 5:19 AM, Yasumasa Suenaga wrote:
>>> Hi David,
>>>
>>> I upload new webrev. Could you review again?
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.03/
>>
>> src/hotspot/share/prims/jvmtiEnv.cpp
>> L1542: // Get stack trace with handshake
>> nit - please add a period at the end.
>
> I will fix it.
>
>
>> L1591: *stack_info_ptr = op.stack_info();
>> The return parameter should not be touched unless the return
>> code in 'err' == JVMTI_ERROR_NONE.
>>
>> old L1582: if (err == JVMTI_ERROR_NONE) {
>> Please restore this check. The return parameter should not
>> be touched unless the return code in 'err' == JVMTI_ERROR_NONE.
>
> I will fix it.
>
>
>> L1272: if (!jt->is_exiting() && (thread_oop != NULL)) {
>> nit - extra parens around the second expression.
>
> I will fix it.
>
>
>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>> old L1532: _result = JVMTI_ERROR_THREAD_NOT_ALIVE;
>> This deletion of the _result field threw me for a minute and
>> then
>> I figured out that the field is init to
>> JVMTI_ERROR_THREAD_NOT_ALIVE
>> in the constructor.
>>
>> L1553: if (!jt->is_exiting() && (jt->threadObj() != NULL)) {
>> nit - extra parens around the second expression.
>
> I will fix it.
>
>
>> src/hotspot/share/prims/jvmtiEnvBase.hpp
>> No comments.
>>
>> src/hotspot/share/runtime/vmOperations.hpp
>> No comments.
>>
>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/GetThreadListStackTraces.java
>>
>> No comments.
>>
>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/OneGetThreadListStackTraces.java
>>
>> L64: startSignal.countDown();
>> I was expecting this to be a call to await() instead of
>> countDown(). What am I missing here?
>>
>> I think this test might be passing by accident right now,
>> but...
>
> Main thread (which call JVMTI functions to test) should wait until
> test thread is ready.
> So main thread would wait startSignal, and test thread would count down.
That's my point. L64 is the main thread so it should be a call to
startSignal.await().
>
>
>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libGetThreadListStackTraces.c
>>
>> L92: jthreads = (jthread *)malloc(sizeof(jthread) * num_threads);
>> You don't check for malloc() failure.
>> 'jthreads' is allocated but never freed.
>
> I will fix it.
>
>
>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.c
>>
>> L91: result = (*jvmti)->SuspendThread(jvmti, thread);
>> Why are you suspending the thread? GetAllStackTraces() and
>> GetThreadListStackTraces() do not require the target thread(s)
>> to be suspend.
>>
>> If you decide not to SuspendThread, then you don't need the
>> AddCapabilities or the ResumeThread calls.
>
> Test thread might not be entered following code (stopSignal.await()).
> We might see deferent call stack between GetAllStackTraces() and
> GetThreadListStackTraces(). We cannot control to freeze call stack of
> test thread in Java code.
> (I didn't use SuspendThread() at first, but I saw some errors which
> causes in above.)
>
> So we need to call SuspendThread() to ensure we can see same call stack.
That sounds like a good reason. Please add a comment near the
SuspendThread()
call so that other readers won't wonder...
Dan
>
>
> Thanks,
>
> Yasumasa
>
>
>> Dan
>>
>>>
>>> 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