RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
David Holmes
david.holmes at oracle.com
Thu Jul 9 08:58:03 UTC 2020
Hi Yasumasa,
On 9/07/2020 10:25 am, Yasumasa Suenaga wrote:
> Hi Dan,
>
> Thanks for your comment!
> I uploaded new webrev:
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.09/
> Diff from previous webrev:
> http://hg.openjdk.java.net/jdk/submit/rev/5d167adf8524
>
> I saw similar build errors in libOneGetThreadListStackTraces.cpp on
> Windows.
> This webrev fixes them.
You shouldn't use %p as it may not be portable. In the VM we use
INTPTR_FORMAT and convert the arg using p2i. I don't know what exists in
the testing code.
David
-----
>
> Thanks,
>
> Yasumasa
>
>
> On 2020/07/09 1:42, Daniel D. Daugherty wrote:
>> > http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.08/
>>
>> src/hotspot/share/prims/jvmtiEnv.cpp
>> No comments.
>>
>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>> L1159: Thread *current_thread = Thread::current();
>> Please add "#ifdef ASSERT" above and "#endif" below since
>> current_thread is only used for the assert() in this function.
>>
>> src/hotspot/share/prims/jvmtiEnvBase.hpp
>> L549: jthread java_thread, jint
>> max_frame_count)
>> L552: _jthread(java_thread),
>> Please: s/java_thread/thread/ on both lines.
>>
>> src/hotspot/share/runtime/vmOperations.hpp
>> No comments.
>>
>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/OneGetThreadListStackTraces.java
>>
>> No comments.
>>
>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
>>
>> L27: #include <errno.h>
>> This include is out of order; should be first in the list.
>>
>> This file doesn't compile on my MBP13:
>>
>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:49:14:
>> error: format specifies type 'unsigned long' but the argument has type
>> 'jmethodID' (aka '_jmethodID *') [-Werror,-Wformat]
>> fi1->method, fi2->method);
>> ^~~~~~~~~~~
>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:49:27:
>> error: format specifies type 'unsigned long' but the argument has type
>> 'jmethodID' (aka '_jmethodID *') [-Werror,-Wformat]
>> fi1->method, fi2->method);
>> ^~~~~~~~~~~
>> 2 errors generated.
>>
>> This change made it compile on my MBP13, but that may break it on
>> other platforms:
>>
>> $ hg diff
>> diff -r 560847c69fbe
>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
>>
>> ---
>> a/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
>> Wed Jul 08 12:13:32 2020 -0400
>> +++
>> b/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
>> Wed Jul 08 12:40:42 2020 -0400
>> @@ -46,7 +46,7 @@
>> if (fi1->method != fi2->method) { /* jvmtiFrameInfo::method */
>> snprintf(err_msg, sizeof(err_msg),
>> "method is different: fi1 = %lx, fi2 = %lx",
>> - fi1->method, fi2->method);
>> + (unsigned long) fi1->method, (unsigned long)
>> fi2->method);
>> env->FatalError(err_msg);
>> } else if (fi1->location != fi2->location) { /*
>> jvmtiFrameInfo::location */
>> snprintf(err_msg, sizeof(err_msg),
>>
>> I'm not sure of the right platform independent way to output
>> the 'method' field.
>>
>> Dan
>>
>>
>> On 7/8/20 4:04 AM, 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
>>>
>>>
>>>> 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