RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
David Holmes
david.holmes at oracle.com
Thu Jul 2 06:05:25 UTC 2020
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().
>
>>>>>> 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.
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
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.
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