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

Patricio Chilano patricio.chilano.mateo at oracle.com
Thu Jul 9 15:00:23 UTC 2020


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.


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