RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
David Holmes
david.holmes at oracle.com
Wed Jul 8 12:29:23 UTC 2020
On 8/07/2020 6:04 pm, Yasumasa Suenaga wrote:
> Hi David,
>
> On 2020/07/08 15:27, David Holmes wrote:
>> Hi Yasumasa,
>>
>> On 7/07/2020 6:54 pm, Yasumasa Suenaga wrote:
>>> Hi David, Serguei,
>>>
>>> Serguei, thank you for replying even though you are on vacaiton!
>>>
>>> I uploaded new webrev:
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.07/
>>> Diff from previous webrev:
>>> http://hg.openjdk.java.net/jdk/submit/rev/77243b1dcbfe
>>>
>>> c'tor of GetSingleStackTraceClosure has jthread argument in this webrev.
>>> Also it does not contain testcase for GetThreadListStackTraces with
>>> all threads, and OneGetThreadListStackTraces would test main thread
>>> only.
>>
>> All those changes are fine in principle for me. One nit/suggestion:
>>
>> src/hotspot/share/prims/jvmtiEnvBase.hpp
>>
>> 544 jthread _java_thread;
>>
>> elsewhere "java_thread" refers to a JavaThread, so to avoid confusion
>> may I suggest this member be named _jthread.
>
> I uploaded new webrev:
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.08/
> Diff from previous webrev:
> http://hg.openjdk.java.net/jdk/submit/rev/ca6263dbdc87
That looks fine.
Thanks,
David
-----
>
>> I'm going to be away for the next couple of days - sorry - but will
>> try to check email on this if I can.
>
> Thanks!
>
>
> Yasumasa
>
>
>> Thanks,
>> David
>> -----
>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/07/07 15:13, David Holmes wrote:
>>>> On 7/07/2020 2:57 pm, Yasumasa Suenaga wrote:
>>>>> Hi David,
>>>>>
>>>>> On 2020/07/07 11:31, David Holmes wrote:
>>>>>> Hi Yasumasa,
>>>>>>
>>>>>> Hard to keep up with the changes - especially without incremental
>>>>>> webrevs.
>>>>>
>>>>> Sorry, I will upload diff from previous webrev in the next.
>>>>>
>>>>>
>>>>>> If GetSingleStackTraceClosure also took the jthread as a
>>>>>> constructor arg, then you wouldn't need to recreate a JNI local
>>>>>> handle when calling _collector.fill_frames. It's a small
>>>>>> simplification and not essential at this stage.
>>>>>
>>>>> I think we should get jthread from an argument of do_thread()
>>>>> because do_thread() would pass the thread which are stopped certainly.
>>>>> It might be simplification if we pass _calling_thread to
>>>>> MultipleStackTracesCollector. `jthread` is only needed to store
>>>>> jvmtiStackInfo.thread . What do you think?
>>>>
>>>> I'm not quite sure what you mean.
>>>>
>>>> I think there is a bit of a design wart with direct handshakes in
>>>> that do_thread takes the target JavaThread as an argument. That's
>>>> useful in a case where you want a HandshakeClosure that can be
>>>> applied to multiple threads, but that's not typically what is needed
>>>> with direct handshakes - there is only a single target. With a
>>>> single-target HandshakeClosure you can capture all the "target"
>>>> information for the operation in the closure instance. So if the
>>>> actual do_thread operation wants the jthread corresponding to the
>>>> target thread then we can store that in the closure rather than
>>>> recomputing it (you could assert it is the same but that seems
>>>> overkill to me).
>>>>
>>>>>
>>>>>> For the test ... I don't see how
>>>>>> Java_GetThreadListStackTraces_checkCallStacks is a valid test. It
>>>>>> gets the stacks of all live threads, then uses that information to
>>>>>> use GetThreadListStackTraces to get the stack for the same set of
>>>>>> threads through a different API. It then compares the two sets of
>>>>>> stacks for each thread expecting them to be the same, but that
>>>>>> need only be the case for the main thread. Other threads could
>>>>>> potentially have a different stack (e.g. if this test is run with
>>>>>> JFR enabled there will be additional threads found.) Further I
>>>>>> would have expected that there already exist tests that check
>>>>>> that, for a given thread (which may be suspended or known to be
>>>>>> blocked) the same stack is found through the two different APIs.
>>>>>
>>>>> vmTestbase/nsk/jvmti/unit/GetAllStackTraces/getallstktr001 would
>>>>> check all of threads via GetThreadListStackTraces() and
>>>>> GetAllStackTraces(), so we might be able to remove
>>>>> GetThreadListStackTraces.java from this webrev.
>>>>
>>>> Yes. The existing test only examines a set of test threads that are
>>>> all blocked on a raw monitor. You do not need to duplicate that test.
>>>>
>>>>> OTOH we don't have testcase for GetThreadListStackTraces() with
>>>>> thread_count == 1, so we need to testcase for it (it is
>>>>> OneGetThreadListStackTraces.java) It would check whether the state
>>>>> of target thread is "waiting" before JNI call to call
>>>>> GetThreadListStackTraces(),
>>>>
>>>> Yes we need to test the special cases introduced by your changes -
>>>> totally agree - and OneGetThreadListStackTraces.java is a good test
>>>> for that.
>>>>
>>>>> and also I expect it would not be run with JFR. (it is not
>>>>> described @run)
>>>>
>>>> The arguments to run with JFR (or a bunch of other things) can be
>>>> passed to the jtreg test harness to be applied to all tests.
>>>>
>>>>> Of course we can check GetThreadListStackTraces() with main thread,
>>>>> but it is not the test for direct handshake for other thread.
>>>>
>>>> Right - that test already exists as per the above.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>> On 6/07/2020 11:29 pm, 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