RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Jul 3 19:32:29 UTC 2020
Hi Yasumasa,
This difference is not that big to care about.
I feel this is really rare case and so, does not worth these complications.
Do we have a real request from customers to optimize it?
Thanks,
Serguei
On 7/3/20 01:16, Yasumasa Suenaga wrote:
> Hi Serguei,
>
> Generally I agree with you, but I have concern about the difference of
> the result of GetStackTrace() and GetThreadListStackTraces().
>
> GetStackTrace: jvmtiFrameInfo
> GetThreadListStackTraces: jvmtiStackInfo
>
> jvmtiStackInfo contains thread state, and it is ensured it is the
> state of the call stack.
> If we want to get both call stack and thread state, we need to suspend
> target thread, and call both GetStackTrace() and GetThreadState(). Is
> it ok?
>
> I was wondering if JDK-8201641 (parent ticket of this change) needed
> them for profiling (dynatrace?)
> If it is responsibility of JVMTI agent implementor, I remove this
> closure.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2020/07/03 16:45, serguei.spitsyn at oracle.com wrote:
>> Hi Yasumasa,
>>
>> After some thinking I've concluded that I do not like this optimization
>> of the GetThreadListStackTraces with GetSingleStackTraceClosure.
>>
>> We may need more opinions on this but these are my points:
>> - it adds some complexity and ugliness
>> - a win is doubtful because it has to be a rare case, so that total
>> overhead should not be high
>> - if it is really high for some use cases then it is up to the user
>> to optimize it with using GetStackTrace instead
>>
>> In such cases with doubtful overhead I usually prefer the simplicity.
>>
>> Good examples where it makes sense to optimize are checks for target
>> thread to be current thread.
>> In such cases there is no need to suspend the target thread, or use a
>> VMop/HandshakeClosure.
>> For instance, please, see the Monitor functions with the check:
>> (java_thread == calling_thread).
>> Getting information for current thread is frequently used case, e.g.
>> to get info at an event point.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 7/2/20 23:29, Yasumasa Suenaga wrote:
>>> Hi Dan, David,
>>>
>>> I uploaded new webrev. Could you review again?
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.04/
>>>
>>> OneGetThreadListStackTraces.java in this webrev would wait until
>>> thread state is transited to "waiting" with spin wait.
>>> CountDownLatch::await call as Dan pointed is fixed in it :)
>>>
>>> Diff from webrev.03:
>>> http://hg.openjdk.java.net/jdk/submit/rev/c9aeb7001e50
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/07/03 14:15, David Holmes wrote:
>>>>
>>>>
>>>> On 3/07/2020 2:27 pm, Yasumasa Suenaga wrote:
>>>>> On 2020/07/03 12:24, Daniel D. Daugherty wrote:
>>>>>> On 7/2/20 10:50 PM, David Holmes wrote:
>>>>>>> Sorry I'm responding here without seeing latest webrev but there
>>>>>>> is enough context I think ...
>>>>>>>
>>>>>>> On 3/07/2020 9:14 am, 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.
>>>>>>>
>>>>>>> But op.stack_info() will return NULL if the error is not
>>>>>>> JVMTI_ERROR_NONE. Are you (Dan) concerned about someone passing
>>>>>>> in a non-null/initialized out-pointer that will be reset to NULL
>>>>>>> if there was an error?
>>>>>>
>>>>>> Actually the way we used to test this in POSIX tests is to call
>>>>>> an API with known bad parameters and the return parameter ptr
>>>>>> set to NULL. If the return parameter ptr was touched when an
>>>>>> error should have been detected on an earlier parameter, then
>>>>>> the test failed.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> 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.
>>>>>>>
>>>>>>> No!
>>>>>>>
>>>>>>> The test thread that previously called obj.wait() now calls
>>>>>>> latch.await().
>>>>>>>
>>>>>>> The main thread that previously called obj.notify() now calls
>>>>>>> latch.countDown().
>>>>>>>
>>>>>>> The main thread continues to spin until it sees the target is
>>>>>>> WAITING before proceeding with the test.
>>>>>
>>>>> If I add spin wait to wait until transit target thread state is
>>>>> WAITING (as following), we don't need to call SuspendThread().
>>>>> Which is better?
>>>>
>>>> The original spin-wait loop checking for WAITING is better because
>>>> it is the only guarantee that the target thread is blocked where
>>>> you need it to be. suspending the thread is racy as you don't know
>>>> exactly where the suspend will hit.
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>>
>>>>> ```
>>>>> /* Wait until the thread state transits to "waiting" */
>>>>> while (th.getState() != Thread.State.WAITING) {
>>>>> Thread.onSpinWait();
>>>>> }
>>>>> ```
>>>>>
>>>>> For simplify, spin wait is prefer to
>>>>> OneGetThreadListStackTraces.java in webrev.03.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>>> Here's the flow as I see it:
>>>>>>
>>>>>> main thread
>>>>>> - start worker thread
>>>>>> - startSignal.await()
>>>>>> - main is now blocked
>>>>>>
>>>>>> worker thread
>>>>>> - startSignal.countDown()
>>>>>> - main is now unblocked
>>>>>> - stopSignal.await()
>>>>>> - worker is now blocked
>>>>>>
>>>>>> main thread
>>>>>> - checkCallStacks(th)
>>>>>> - stopSignal.countDown()
>>>>>> - worker is now unblocked
>>>>>> - th.join
>>>>>> - main is now blocked
>>>>>>
>>>>>> worker thread
>>>>>> - runs off the end of run()
>>>>>> - main is now unblocked
>>>>>>
>>>>>> main thread
>>>>>> - run off the end of main()
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> 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.
>>>>>>>
>>>>>>> If you are checking that the thread is in state WAITING then it
>>>>>>> cannot escape from that state and you can sample the stack
>>>>>>> multiple times from any API and get the same result.
>>>>>>>
>>>>>>> I suspect the errors you saw were from the apparent incorrect
>>>>>>> use of the CountDownLatch.
>>>>>>
>>>>>> With the flow outlined above, the worker thread should be
>>>>>> nicely blocked in stopSignal.await() when stuff is sampled.
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Cheers,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>>
>>>>>>>> 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