RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
David Holmes
david.holmes at oracle.com
Wed Jul 8 06:27:51 UTC 2020
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'm going to be away for the next couple of days - sorry - but will try
to check email on this if I can.
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