RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Jul 7 07:43:29 UTC 2020
Hi Yasumasa,
Thank you for this update - the test looks much better and is readable now.
I'm on vacation this week but will try to look at your fixes a little
bit more.
As I understand you are going to post one more update.
Thanks,
Serguei
On 7/6/20 06:29, Yasumasa Suenaga wrote:
> Hi Serguei,
>
> Thanks for your comment!
>
> I think C++ is more simple to implement the test agent as you said.
> So I implement it in C++ in new webrev. Could you review again?
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.06/
>
> Also I refactored libGetThreadListStackTraces.cpp, and I've kept
> exception check after IsSameObject().
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2020/07/06 16:32, serguei.spitsyn at oracle.com wrote:
>> Hi Yasumasa,
>>
>> Thank you for the update.
>> I think, a pending exception after IsSameObject needs to be checked.
>>
>> The checkStackInfo() needs one more refactoring as I've already
>> suggested.
>> The body of the loop at L68-L78 should be converted to a function
>> check_frame_info.
>> The si1->frame_buffer[i] and si2->frame_buffer[i] will be passed as
>> fi1 and fi2.
>> The index can be passed as well.
>> I'm still suggesting to simplify the local exception_msg to something
>> shorter like err_msg or exc_msg.
>>
>> I'm not sure using fatal is right here:
>>
>> This fragment looks strange:
>>
>> 152 if ((*env)->IsSameObject(env, stack_info[i].thread, thread)) {
>> 153 target_info = &stack_info[i];
>> 154 break;
>> 155 } else if ((*env)->ExceptionOccurred(env)) {
>> 156 (*env)->ExceptionDescribe(env);
>> 157 (*env)->FatalError(env, __FILE__);
>> 158 }
>>
>> I expected it to be:
>>
>> jboolean same = (*env)->IsSameObject(env, stack_info[i].thread,
>> thread);
>> if ((*env)->ExceptionOccurred(env)) {
>> (*env)->ExceptionDescribe(env);
>> (*env)->FatalError(env, __FILE__);
>> }
>> if (same) {
>> target_info = &stack_info[i];
>> break;
>> }
>>
>> Would it better to port this agent to C++ to simplify this code nicer?
>>
>> Thanks,
>> Serguei
>>
>>
>> On 7/5/20 06:13, Yasumasa Suenaga wrote:
>>> Hi Serguei,
>>>
>>> Thanks for your comment!
>>> I refactored testcase. Could you review again?
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.05/
>>>
>>> It would check Java exception after IsSameObject() call. Does it need?
>>> Any exceptions are not described in JNI document[1], and JNI
>>> implementation (jni_IsSameObject()) does not seem to throw it.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> [1]
>>> https://docs.oracle.com/en/java/javase/14/docs/specs/jni/functions.html#issameobject
>>>
>>>
>>> On 2020/07/05 14:46, serguei.spitsyn at oracle.com wrote:
>>>> Hi Yasumasa,
>>>>
>>>>
>>>> Okay, thanks.
>>>> Then I'm okay to keep the GetSingleStackTraceClosure.
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.04/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libGetThreadListStackTraces.c.html
>>>>
>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.04/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.c.html
>>>>
>>>>
>>>> I'm not sure the function 'is_same_thread() is needed.
>>>> Why do not use the JNI IsSameObject instead?
>>>>
>>>> It seems to be a typo at L132 and L137.
>>>> You, probably. did not want to print the same information for
>>>> stack_info_1[i].frame_buffer[j].XXX twice.
>>>>
>>>> The code at lines 112-142 is not readable.
>>>> I'd suggest to make a couple of refactoring steps.
>>>>
>>>> First step to simplify this a little bit would be with some
>>>> renaming and getting rid of indexes:
>>>>
>>>> 71 char err_msg[EXCEPTION_MSG_LEN] = {0};
>>>> ...
>>>> 112 /* Iterate all jvmtiStackInfo to check */
>>>> 113 for (i = 0; i < num_threads, *exception_msg != '\0'; i++) {
>>>> jvmtiStackInfo *si1 = stack_info_1[i];
>>>> jvmtiStackInfo *si2 = stack_info_2[i];
>>>> 114 if (!IsSameObject(env, si1.thread, si2.thread)) { /*
>>>> jvmtiStackInfo::thread */
>>>> 115 snprintf(err_msg, sizeof(err_msg),
>>>> 116 "thread[%d] is different: stack_info_1 = %p,
>>>> stack_info_2 = %p",
>>>> 117 i, sinfo1.thread, sinfo2.thread);
>>>> 118 } else if (si1.state != si2.state) { /*
>>>> jvmtiStackInfo::state */
>>>> 119 snprintf(err_msg, sizeof(err_msg),
>>>> 120 "state[%d] is different: stack_info_1 = %d,
>>>> stack_info_2 = %d",
>>>> 121 i, si1.state, si2.state);
>>>> 122 } else if (si1.frame_count != si2.frame_count) { /*
>>>> jvmtiStackInfo::frame_count */
>>>> 123 snprintf(err_msg, sizeof(err_msg),
>>>> 124 "frame_count[%d] is different: stack_info_1 =
>>>> %d, stack_info_2 = %d",
>>>> 125 i, si1.frame_count, si2.frame_count);
>>>> 126 } else {
>>>> 127 /* Iterate all jvmtiFrameInfo to check */
>>>> 128 for (j = 0; j < si1.frame_count; j++) {
>>>> 129 if (si1.frame_buffer[j].method !=
>>>> si1.frame_buffer[j].method) { /* jvmtiFrameInfo::method */
>>>> 130 snprintf(err_msg, sizeof(err_msg),
>>>> 131 "thread [%d] frame_buffer[%d].method is
>>>> different: stack_info_1 = %lx, stack_info_2 = %lx",
>>>> 132 i, j, si1.frame_buffer[j].method,
>>>> si2.frame_buffer[j].method);
>>>> 133 break;
>>>> 134 } else if (si1.frame_buffer[j].location !=
>>>> si1.frame_buffer[j].location) { /* jvmtiFrameInfo::location */
>>>> 135 snprintf(err_msg, sizeof(err_msg),
>>>> 136 "thread [%d] frame_buffer[%d].location is
>>>> different: stack_info_1 = %ld, stack_info_2 = %ld",
>>>> 137 i, j, si1.frame_buffer[j].location,
>>>> si2.frame_buffer[j].location);
>>>> 138 break;
>>>> 139 }
>>>> 140 }
>>>> 141 }
>>>> 142 }
>>>>
>>>> Another step would be to create functions that implement a body of
>>>> each loop.
>>>> You can use the same techniques to simplify similar place
>>>> (L127-L138) in the libOneGetThreadListStackTraces.c.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 7/3/20 15:55, Yasumasa Suenaga wrote:
>>>>> Hi Serguei,
>>>>>
>>>>> I'm not an Oracle employee, so I cannot know real request(s) from
>>>>> your customers.
>>>>> However JDK-8201641 says Dynatrace has requested this enhancement.
>>>>>
>>>>> BTW I haven't heared any request from my customers about this.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2020/07/04 4:32, serguei.spitsyn at oracle.com wrote:
>>>>>> 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