RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake

David Holmes david.holmes at oracle.com
Sat Jul 11 13:50:12 UTC 2020


On 11/07/2020 11:52 am, Yasumasa Suenaga wrote:
> Thanks Dan!
> 
> David, Serguei, are you ok to this change?

Yes it seems fine.

In relation to an earlier comment:

 > I replaced %p to %lx, and also cast values to unsigned long

Never use long or unsigned long as they can be different sizes on 
different platforms (specifically long is not 64-bit on Windows).

Thanks,
David
-----

> 
> Yasumasa
> 
> 
> On 2020/07/11 7:00, Daniel D. Daugherty wrote:
>> On 7/10/20 3:29 AM, Yasumasa Suenaga wrote:
>>> Thanks Patricio!
>>>
>>> I uploaded new webrev. Could you review again?
>>> It passed all tests on submit repo, and passed jtreg tests about JVMTI.
>>>
>>>   http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.10/
>>>   Diff from webrev.09: 
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.10/incremental/
>>
>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp 
>>
>>      No comments.
>>
>> Thumbs up on the incremental.
>>
>> Dan
>>
>>
>>>
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/07/10 14:49, Patricio Chilano wrote:
>>>> Hi Yasumasa,
>>>>
>>>> On 7/10/20 1:58 AM, Yasumasa Suenaga wrote:
>>>>> Hi Patricio,
>>>>>
>>>>> Thanks for your advice!
>>>>> I've fixed testcase as [1], but I still see an error in 
>>>>> validate-headers-linux-x64-build-1 on submit repo.
>>>>> What does it mean? Can you share details?
>>>>>
>>>>>   mach5-one-ysuenaga-JDK-8242428-20200710-0339-12529134
>>>> Not your bug, you are getting the header validation failure from 
>>>> 8248570. It was fixed two days ago so just update the repo and it 
>>>> should work now.
>>>>
>>>> Thanks,
>>>> Patricio
>>>>> Of course this change can be built on my Linux box (Fedora 32, 
>>>>> AMD64, GCC 10.1)
>>>>>
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> [1] 
>>>>> http://hg.openjdk.java.net/jdk/submit/file/45f52142db9d/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp 
>>>>>
>>>>>
>>>>>
>>>>> On 2020/07/10 2:18, Patricio Chilano wrote:
>>>>>>
>>>>>> 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