RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Patricio Chilano
patricio.chilano.mateo at oracle.com
Thu Jul 9 17:18:31 UTC 2020
On 7/9/20 12:00 PM, Patricio Chilano wrote:
> Hi Yasumasa,
>
> On 7/9/20 9:30 AM, Yasumasa Suenaga wrote:
>> On 2020/07/09 17:58, David Holmes wrote:
>>> 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.
>>
>> I replaced %p to %lx, and also cast values to unsigned long [1] [2],
>> but the test on submit repo was failed.
>> Can anyone share details of
>> mach5-one-ysuenaga-JDK-8242428-20200709-1030-12500928 ?
> These are the errors I see for the macOS build:
>
> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:53:14:
> error: format specifies type 'long long' but the argument has type
> 'jlocation' (aka 'long') [-Werror,-Wformat]
> fi1->location, fi2->location);
> ^~~~~~~~~~~~~
> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:53:29:
> error: format specifies type 'long long' but the argument has type
> 'jlocation' (aka 'long') [-Werror,-Wformat]
> fi1->location, fi2->location);
> ^~~~~~~~~~~~~
>
> These are the ones I see for the Windows build:
>
> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(48):
> warning C4311: 'type cast': pointer truncation from 'jmethodID' to
> 'unsigned long'
> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(48):
> warning C4302: 'type cast': truncation from 'jmethodID' to 'unsigned
> long'
> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(48):
> warning C4311: 'type cast': pointer truncation from 'jmethodID' to
> 'unsigned long'
> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(48):
> warning C4302: 'type cast': truncation from 'jmethodID' to 'unsigned
> long'
> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(70):
> warning C4311: 'type cast': pointer truncation from 'jthread' to
> 'unsigned long'
> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(70):
> warning C4302: 'type cast': truncation from 'jthread' to 'unsigned long'
> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(70):
> warning C4311: 'type cast': pointer truncation from 'jthread' to
> 'unsigned long'
> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(70):
> warning C4302: 'type cast': truncation from 'jthread' to 'unsigned long'
>
>
> You will probably want to use the macros defined in
> src/hotspot/share/utilities/globalDefinitions.hpp. Let me know if you
> need me to test something.
With these changes the build works okay on Linux, Windows and macOS:
---
a/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
+++
b/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
@@ -27,4 +27,5 @@
#include <stdio.h>
#include <stdlib.h>
+#include <inttypes.h>
#define MAX_FRAMES 100
@@ -45,11 +46,11 @@
if (fi1->method != fi2->method) { /* jvmtiFrameInfo::method */
snprintf(err_msg, sizeof(err_msg),
- "method is different: fi1 = %p, fi2 = %p",
- fi1->method, fi2->method);
+ "method is different: fi1 = 0x%016" PRIxPTR " , fi2 =
0x%016" PRIxPTR,
+ (intptr_t)fi1->method, (intptr_t)fi2->method);
env->FatalError(err_msg);
} else if (fi1->location != fi2->location) { /*
jvmtiFrameInfo::location */
snprintf(err_msg, sizeof(err_msg),
- "location is different: fi1 = %lld, fi2 = %lld",
- fi1->location, fi2->location);
+ "location is different: fi1 = %" PRId64 " , fi2 = %" PRId64,
+ (int64_t)fi1->location, (int64_t)fi2->location);
env->FatalError(err_msg);
}
@@ -67,5 +68,5 @@
if (!is_same) { /* jvmtiStackInfo::thread */
snprintf(err_msg, sizeof(err_msg),
- "thread is different: si1 = %p, si2 = %p", si1->thread,
si2->thread);
+ "thread is different: si1 = 0x%016" PRIxPTR " , si2 =
0x%016" PRIxPTR, (intptr_t)si1->thread, (intptr_t)si2->thread);
env->FatalError(err_msg);
} else if (si1->state != si2->state) { /* jvmtiStackInfo::state */
Maybe you can use something like that.
Thanks,
Patricio
> Thanks,
> Patricio
>> Thanks,
>>
>> Yasumasa
>>
>>
>> [1] http://hg.openjdk.java.net/jdk/submit/rev/dfca51958217
>> [2] http://hg.openjdk.java.net/jdk/submit/rev/3665361fa91b
>>
>>
>>> 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