RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Jul 2 22:16:54 UTC 2020
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.
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.
L1272: if (!jt->is_exiting() && (thread_oop != NULL)) {
nit - extra parens around the second expression.
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.
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...
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.
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.
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