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

David Holmes david.holmes at oracle.com
Thu Jul 9 08:58:03 UTC 2020


Hi Yasumasa,

On 9/07/2020 10:25 am, Yasumasa Suenaga wrote:
> Hi Dan,
> 
> Thanks for your comment!
> I uploaded new webrev:
> 
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.09/
>    Diff from previous webrev: 
> http://hg.openjdk.java.net/jdk/submit/rev/5d167adf8524
> 
> I saw similar build errors in libOneGetThreadListStackTraces.cpp on 
> Windows.
> This webrev fixes them.

You shouldn't use %p as it may not be portable. In the VM we use 
INTPTR_FORMAT and convert the arg using p2i. I don't know what exists in 
the testing code.

David
-----

> 
> Thanks,
> 
> Yasumasa
> 
> 
> On 2020/07/09 1:42, Daniel D. Daugherty wrote:
>>  > http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.08/
>>
>> src/hotspot/share/prims/jvmtiEnv.cpp
>>      No comments.
>>
>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>>      L1159:   Thread *current_thread = Thread::current();
>>          Please add "#ifdef ASSERT" above and "#endif" below since
>>          current_thread is only used for the assert() in this function.
>>
>> src/hotspot/share/prims/jvmtiEnvBase.hpp
>>      L549:                              jthread java_thread, jint 
>> max_frame_count)
>>      L552:       _jthread(java_thread),
>>          Please: s/java_thread/thread/ on both lines.
>>
>> src/hotspot/share/runtime/vmOperations.hpp
>>      No comments.
>>
>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/OneGetThreadListStackTraces.java 
>>
>>      No comments.
>>
>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp 
>>
>>      L27: #include <errno.h>
>>          This include is out of order; should be first in the list.
>>
>>      This file doesn't compile on my MBP13:
>>
>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:49:14: 
>> error: format specifies type 'unsigned long' but the argument has type 
>> 'jmethodID' (aka '_jmethodID *') [-Werror,-Wformat]
>>               fi1->method, fi2->method);
>>               ^~~~~~~~~~~
>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:49:27: 
>> error: format specifies type 'unsigned long' but the argument has type 
>> 'jmethodID' (aka '_jmethodID *') [-Werror,-Wformat]
>>               fi1->method, fi2->method);
>>                            ^~~~~~~~~~~
>>      2 errors generated.
>>
>>      This change made it compile on my MBP13, but that may break it on
>>      other platforms:
>>
>>      $ hg diff
>>      diff -r 560847c69fbe 
>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp 
>>
>>      --- 
>> a/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp 
>> Wed Jul 08 12:13:32 2020 -0400
>>      +++ 
>> b/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp 
>> Wed Jul 08 12:40:42 2020 -0400
>>      @@ -46,7 +46,7 @@
>>         if (fi1->method != fi2->method) { /* jvmtiFrameInfo::method */
>>           snprintf(err_msg, sizeof(err_msg),
>>                    "method is different: fi1 = %lx, fi2 = %lx",
>>      -             fi1->method, fi2->method);
>>      +             (unsigned long) fi1->method, (unsigned long) 
>> fi2->method);
>>           env->FatalError(err_msg);
>>         } else if (fi1->location != fi2->location) { /* 
>> jvmtiFrameInfo::location */
>>           snprintf(err_msg, sizeof(err_msg),
>>
>>      I'm not sure of the right platform independent way to output
>>      the 'method' field.
>>
>> Dan
>>
>>
>> On 7/8/20 4:04 AM, Yasumasa Suenaga wrote:
>>> Hi David,
>>>
>>> On 2020/07/08 15:27, David Holmes wrote:
>>>> Hi Yasumasa,
>>>>
>>>> On 7/07/2020 6:54 pm, Yasumasa Suenaga wrote:
>>>>> Hi David, Serguei,
>>>>>
>>>>> Serguei, thank you for replying even though you are on vacaiton!
>>>>>
>>>>> I uploaded new webrev:
>>>>>
>>>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.07/
>>>>>    Diff from previous webrev: 
>>>>> http://hg.openjdk.java.net/jdk/submit/rev/77243b1dcbfe
>>>>>
>>>>> c'tor of GetSingleStackTraceClosure has jthread argument in this 
>>>>> webrev.
>>>>> Also it does not contain testcase for GetThreadListStackTraces with 
>>>>> all threads, and OneGetThreadListStackTraces would test main thread 
>>>>> only.
>>>>
>>>> All those changes are fine in principle for me. One nit/suggestion:
>>>>
>>>> src/hotspot/share/prims/jvmtiEnvBase.hpp
>>>>
>>>>   544   jthread _java_thread;
>>>>
>>>> elsewhere "java_thread" refers to a JavaThread, so to avoid 
>>>> confusion may I suggest this member be named _jthread.
>>>
>>> I uploaded new webrev:
>>>
>>>   http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.08/
>>>   Diff from previous webrev: 
>>> http://hg.openjdk.java.net/jdk/submit/rev/ca6263dbdc87
>>>
>>>
>>>> I'm going to be away for the next couple of days - sorry - but will 
>>>> try to check email on this if I can.
>>>
>>> Thanks!
>>>
>>>
>>> Yasumasa
>>>
>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2020/07/07 15:13, David Holmes wrote:
>>>>>> On 7/07/2020 2:57 pm, Yasumasa Suenaga wrote:
>>>>>>> Hi David,
>>>>>>>
>>>>>>> On 2020/07/07 11:31, David Holmes wrote:
>>>>>>>> Hi Yasumasa,
>>>>>>>>
>>>>>>>> Hard to keep up with the changes - especially without 
>>>>>>>> incremental webrevs.
>>>>>>>
>>>>>>> Sorry, I will upload diff from previous webrev in the next.
>>>>>>>
>>>>>>>
>>>>>>>> If GetSingleStackTraceClosure also took the jthread as a 
>>>>>>>> constructor arg, then you wouldn't need to recreate a JNI local 
>>>>>>>> handle when calling _collector.fill_frames. It's a small 
>>>>>>>> simplification and not essential at this stage.
>>>>>>>
>>>>>>> I think we should get jthread from an argument of do_thread() 
>>>>>>> because do_thread() would pass the thread which are stopped 
>>>>>>> certainly.
>>>>>>> It might be simplification if we pass _calling_thread to 
>>>>>>> MultipleStackTracesCollector. `jthread` is only needed to store 
>>>>>>> jvmtiStackInfo.thread . What do you think?
>>>>>>
>>>>>> I'm not quite sure what you mean.
>>>>>>
>>>>>> I think there is a bit of a design wart with direct handshakes in 
>>>>>> that do_thread takes the target JavaThread as an argument. That's 
>>>>>> useful in a case where you want a HandshakeClosure that can be 
>>>>>> applied to multiple threads, but that's not typically what is 
>>>>>> needed with direct handshakes - there is only a single target. 
>>>>>> With a single-target HandshakeClosure you can capture all the 
>>>>>> "target" information for the operation in the closure instance. So 
>>>>>> if the actual do_thread operation wants the jthread corresponding 
>>>>>> to the target thread then we can store that in the closure rather 
>>>>>> than recomputing it (you could assert it is the same but that 
>>>>>> seems overkill to me).
>>>>>>
>>>>>>>
>>>>>>>> For the test ... I don't see how 
>>>>>>>> Java_GetThreadListStackTraces_checkCallStacks is a valid test. 
>>>>>>>> It gets the stacks of all live threads, then uses that 
>>>>>>>> information to use GetThreadListStackTraces to get the stack for 
>>>>>>>> the same set of threads through a different API. It then 
>>>>>>>> compares the two sets of stacks for each thread expecting them 
>>>>>>>> to be the same, but that need only be the case for the main 
>>>>>>>> thread. Other threads could potentially have a different stack 
>>>>>>>> (e.g. if this test is run with JFR enabled there will be 
>>>>>>>> additional threads found.) Further I would have expected that 
>>>>>>>> there already exist tests that check that, for a given thread 
>>>>>>>> (which may be suspended or known to be blocked) the same stack 
>>>>>>>> is found through the two different APIs.
>>>>>>>
>>>>>>> vmTestbase/nsk/jvmti/unit/GetAllStackTraces/getallstktr001 would 
>>>>>>> check all of threads via GetThreadListStackTraces() and 
>>>>>>> GetAllStackTraces(), so we might be able to remove 
>>>>>>> GetThreadListStackTraces.java from this webrev.
>>>>>>
>>>>>> Yes. The existing test only examines a set of test threads that 
>>>>>> are all blocked on a raw monitor. You do not need to duplicate 
>>>>>> that test.
>>>>>>
>>>>>>> OTOH we don't have testcase for GetThreadListStackTraces() with 
>>>>>>> thread_count == 1, so we need to testcase for it (it is 
>>>>>>> OneGetThreadListStackTraces.java) It would check whether the 
>>>>>>> state of target thread is "waiting" before JNI call to call 
>>>>>>> GetThreadListStackTraces(),
>>>>>>
>>>>>> Yes we need to test the special cases introduced by your changes - 
>>>>>> totally agree - and OneGetThreadListStackTraces.java is a good 
>>>>>> test for that.
>>>>>>
>>>>>>> and also I expect it would not be run with JFR. (it is not 
>>>>>>> described @run)
>>>>>>
>>>>>> The arguments to run with JFR (or a bunch of other things) can be 
>>>>>> passed to the jtreg test harness to be applied to all tests.
>>>>>>
>>>>>>> Of course we can check GetThreadListStackTraces() with main 
>>>>>>> thread, but it is not the test for direct handshake for other 
>>>>>>> thread.
>>>>>>
>>>>>> Right - that test already exists as per the above.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
>>>>>>>> On 6/07/2020 11:29 pm, Yasumasa Suenaga wrote:
>>>>>>>>> Hi Serguei,
>>>>>>>>>
>>>>>>>>> Thanks for your comment!
>>>>>>>>>
>>>>>>>>> I think C++ is more simple to implement the test agent as you 
>>>>>>>>> said.
>>>>>>>>> So I implement it in C++ in new webrev. Could you review again?
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.06/
>>>>>>>>>
>>>>>>>>> Also I refactored libGetThreadListStackTraces.cpp, and I've 
>>>>>>>>> kept exception check after IsSameObject().
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2020/07/06 16:32, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>
>>>>>>>>>> Thank you for the update.
>>>>>>>>>> I think, a pending exception after IsSameObject needs to be 
>>>>>>>>>> checked.
>>>>>>>>>>
>>>>>>>>>> The checkStackInfo() needs one more refactoring as I've 
>>>>>>>>>> already suggested.
>>>>>>>>>> The body of the loop at L68-L78 should be converted to a 
>>>>>>>>>> function check_frame_info.
>>>>>>>>>> The si1->frame_buffer[i] and si2->frame_buffer[i] will be 
>>>>>>>>>> passed as fi1 and fi2.
>>>>>>>>>> The index can be passed as well.
>>>>>>>>>> I'm still suggesting to simplify the local exception_msg to 
>>>>>>>>>> something shorter like err_msg or exc_msg.
>>>>>>>>>>
>>>>>>>>>> I'm not sure using fatal is right here:
>>>>>>>>>>
>>>>>>>>>> This fragment looks strange:
>>>>>>>>>>
>>>>>>>>>>   152     if ((*env)->IsSameObject(env, stack_info[i].thread, 
>>>>>>>>>> thread)) {
>>>>>>>>>>   153       target_info = &stack_info[i];
>>>>>>>>>>   154       break;
>>>>>>>>>>   155     } else if ((*env)->ExceptionOccurred(env)) {
>>>>>>>>>>   156       (*env)->ExceptionDescribe(env);
>>>>>>>>>>   157       (*env)->FatalError(env, __FILE__);
>>>>>>>>>>   158     }
>>>>>>>>>>
>>>>>>>>>> I expected it to be:
>>>>>>>>>>
>>>>>>>>>>     jboolean same = (*env)->IsSameObject(env, 
>>>>>>>>>> stack_info[i].thread, thread);
>>>>>>>>>>     if ((*env)->ExceptionOccurred(env)) {
>>>>>>>>>>       (*env)->ExceptionDescribe(env);
>>>>>>>>>>       (*env)->FatalError(env, __FILE__);
>>>>>>>>>>     }
>>>>>>>>>>     if (same) {
>>>>>>>>>>       target_info = &stack_info[i];
>>>>>>>>>>       break;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>> Would it better to port this agent to C++ to simplify this 
>>>>>>>>>> code nicer?
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Serguei
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 7/5/20 06:13, Yasumasa Suenaga wrote:
>>>>>>>>>>> Hi Serguei,
>>>>>>>>>>>
>>>>>>>>>>> Thanks for your comment!
>>>>>>>>>>> I refactored testcase. Could you review again?
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.05/
>>>>>>>>>>>
>>>>>>>>>>> It would check Java exception after IsSameObject() call. Does 
>>>>>>>>>>> it need?
>>>>>>>>>>> Any exceptions are not described in JNI document[1], and JNI 
>>>>>>>>>>> implementation (jni_IsSameObject()) does not seem to throw it.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> Yasumasa
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> [1] 
>>>>>>>>>>> https://docs.oracle.com/en/java/javase/14/docs/specs/jni/functions.html#issameobject 
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2020/07/05 14:46, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Okay, thanks.
>>>>>>>>>>>> Then I'm okay to keep the GetSingleStackTraceClosure.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.04/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libGetThreadListStackTraces.c.html 
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.04/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.c.html 
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I'm not sure the function 'is_same_thread() is needed.
>>>>>>>>>>>> Why do not use the JNI IsSameObject instead?
>>>>>>>>>>>>
>>>>>>>>>>>> It seems to be a typo at L132 and L137.
>>>>>>>>>>>> You, probably. did not want to print the same information 
>>>>>>>>>>>> for stack_info_1[i].frame_buffer[j].XXX twice.
>>>>>>>>>>>>
>>>>>>>>>>>> The code at lines 112-142 is not readable.
>>>>>>>>>>>> I'd suggest to make a couple of refactoring steps.
>>>>>>>>>>>>
>>>>>>>>>>>> First step to simplify this a little bit would be with some 
>>>>>>>>>>>> renaming and getting rid of indexes:
>>>>>>>>>>>>
>>>>>>>>>>>>    71 char err_msg[EXCEPTION_MSG_LEN] = {0};
>>>>>>>>>>>>   ...
>>>>>>>>>>>>   112   /* Iterate all jvmtiStackInfo to check */
>>>>>>>>>>>>   113   for (i = 0; i < num_threads, *exception_msg != '\0'; 
>>>>>>>>>>>> i++) {
>>>>>>>>>>>>           jvmtiStackInfo *si1 = stack_info_1[i];
>>>>>>>>>>>>           jvmtiStackInfo *si2 = stack_info_2[i];
>>>>>>>>>>>>   114     if (!IsSameObject(env, si1.thread, si2.thread)) { 
>>>>>>>>>>>> /* jvmtiStackInfo::thread */
>>>>>>>>>>>>   115       snprintf(err_msg, sizeof(err_msg),
>>>>>>>>>>>>   116                "thread[%d] is different: stack_info_1 
>>>>>>>>>>>> = %p, stack_info_2 = %p",
>>>>>>>>>>>>   117                i, sinfo1.thread, sinfo2.thread);
>>>>>>>>>>>>   118     } else if (si1.state != si2.state) { /* 
>>>>>>>>>>>> jvmtiStackInfo::state */
>>>>>>>>>>>>   119       snprintf(err_msg, sizeof(err_msg),
>>>>>>>>>>>>   120                "state[%d] is different: stack_info_1 = 
>>>>>>>>>>>> %d, stack_info_2 = %d",
>>>>>>>>>>>>   121                i, si1.state, si2.state);
>>>>>>>>>>>>   122     } else if (si1.frame_count != si2.frame_count) { 
>>>>>>>>>>>> /* jvmtiStackInfo::frame_count */
>>>>>>>>>>>>   123       snprintf(err_msg, sizeof(err_msg),
>>>>>>>>>>>>   124                "frame_count[%d] is different: 
>>>>>>>>>>>> stack_info_1 = %d, stack_info_2 = %d",
>>>>>>>>>>>>   125                i, si1.frame_count, si2.frame_count);
>>>>>>>>>>>>   126     } else {
>>>>>>>>>>>>   127       /* Iterate all jvmtiFrameInfo to check */
>>>>>>>>>>>>   128       for (j = 0; j < si1.frame_count; j++) {
>>>>>>>>>>>>   129         if (si1.frame_buffer[j].method != 
>>>>>>>>>>>> si1.frame_buffer[j].method) { /* jvmtiFrameInfo::method */
>>>>>>>>>>>>   130           snprintf(err_msg, sizeof(err_msg),
>>>>>>>>>>>>   131                    "thread [%d] 
>>>>>>>>>>>> frame_buffer[%d].method is different: stack_info_1 = %lx, 
>>>>>>>>>>>> stack_info_2 = %lx",
>>>>>>>>>>>>   132                    i, j, si1.frame_buffer[j].method, 
>>>>>>>>>>>> si2.frame_buffer[j].method);
>>>>>>>>>>>>   133           break;
>>>>>>>>>>>>   134         } else if (si1.frame_buffer[j].location != 
>>>>>>>>>>>> si1.frame_buffer[j].location) { /* jvmtiFrameInfo::location */
>>>>>>>>>>>>   135           snprintf(err_msg, sizeof(err_msg),
>>>>>>>>>>>>   136                    "thread [%d] 
>>>>>>>>>>>> frame_buffer[%d].location is different: stack_info_1 = %ld, 
>>>>>>>>>>>> stack_info_2 = %ld",
>>>>>>>>>>>>   137                    i, j, si1.frame_buffer[j].location, 
>>>>>>>>>>>> si2.frame_buffer[j].location);
>>>>>>>>>>>>   138           break;
>>>>>>>>>>>>   139         }
>>>>>>>>>>>>   140       }
>>>>>>>>>>>>   141     }
>>>>>>>>>>>>   142   }
>>>>>>>>>>>>
>>>>>>>>>>>> Another step would be to create functions that implement a 
>>>>>>>>>>>> body of each loop.
>>>>>>>>>>>> You can use the same techniques to simplify similar place 
>>>>>>>>>>>> (L127-L138) in the libOneGetThreadListStackTraces.c.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Serguei
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 7/3/20 15:55, Yasumasa Suenaga wrote:
>>>>>>>>>>>>> Hi Serguei,
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'm not an Oracle employee, so I cannot know real 
>>>>>>>>>>>>> request(s) from your customers.
>>>>>>>>>>>>> However JDK-8201641 says Dynatrace has requested this 
>>>>>>>>>>>>> enhancement.
>>>>>>>>>>>>>
>>>>>>>>>>>>> BTW I haven't heared any request from my customers about this.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2020/07/04 4:32, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This difference is not that big to care about.
>>>>>>>>>>>>>> I feel this is really rare case and so, does not worth 
>>>>>>>>>>>>>> these complications.
>>>>>>>>>>>>>> Do we have a real request from customers to optimize it?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Serguei
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 7/3/20 01:16, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>> Hi Serguei,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Generally I agree with you, but I have concern about the 
>>>>>>>>>>>>>>> difference of the result of GetStackTrace() and 
>>>>>>>>>>>>>>> GetThreadListStackTraces().
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>   GetStackTrace: jvmtiFrameInfo
>>>>>>>>>>>>>>>   GetThreadListStackTraces: jvmtiStackInfo
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> jvmtiStackInfo contains thread state, and it is ensured 
>>>>>>>>>>>>>>> it is the state of the call stack.
>>>>>>>>>>>>>>> If we want to get both call stack and thread state, we 
>>>>>>>>>>>>>>> need to suspend target thread, and call both 
>>>>>>>>>>>>>>> GetStackTrace() and GetThreadState(). Is it ok?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I was wondering if JDK-8201641 (parent ticket of this 
>>>>>>>>>>>>>>> change) needed them for profiling (dynatrace?)
>>>>>>>>>>>>>>> If it is responsibility of JVMTI agent implementor, I 
>>>>>>>>>>>>>>> remove this closure.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 2020/07/03 16:45, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> After some thinking I've concluded that I do not like 
>>>>>>>>>>>>>>>> this optimization
>>>>>>>>>>>>>>>> of the GetThreadListStackTraces with 
>>>>>>>>>>>>>>>> GetSingleStackTraceClosure.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> We may need more opinions on this but these are my points:
>>>>>>>>>>>>>>>>   - it adds some complexity and ugliness
>>>>>>>>>>>>>>>>   - a win is doubtful because it has to be a rare case, 
>>>>>>>>>>>>>>>> so that total overhead should not be high
>>>>>>>>>>>>>>>>   - if it is really high for some use cases then it is 
>>>>>>>>>>>>>>>> up to the user
>>>>>>>>>>>>>>>>     to optimize it with using GetStackTrace instead
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In such cases with doubtful overhead I usually prefer 
>>>>>>>>>>>>>>>> the simplicity.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Good examples where it makes sense to optimize are 
>>>>>>>>>>>>>>>> checks for target thread to be current thread.
>>>>>>>>>>>>>>>> In such cases there is no need to suspend the target 
>>>>>>>>>>>>>>>> thread, or use a VMop/HandshakeClosure.
>>>>>>>>>>>>>>>> For instance, please, see the Monitor functions with the 
>>>>>>>>>>>>>>>> check: (java_thread == calling_thread).
>>>>>>>>>>>>>>>> Getting information for current thread is frequently 
>>>>>>>>>>>>>>>> used case, e.g. to get info at an event point.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>> Serguei
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 7/2/20 23:29, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>> Hi Dan, David,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I uploaded new webrev. Could you review again?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.04/ 
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> OneGetThreadListStackTraces.java in this webrev would 
>>>>>>>>>>>>>>>>> wait until thread state is transited to "waiting" with 
>>>>>>>>>>>>>>>>> spin wait.
>>>>>>>>>>>>>>>>> CountDownLatch::await call as Dan pointed is fixed in 
>>>>>>>>>>>>>>>>> it :)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Diff from webrev.03:
>>>>>>>>>>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/c9aeb7001e50
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 2020/07/03 14:15, David Holmes wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 3/07/2020 2:27 pm, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>> On 2020/07/03 12:24, Daniel D. Daugherty wrote:
>>>>>>>>>>>>>>>>>>>> On 7/2/20 10:50 PM, David Holmes wrote:
>>>>>>>>>>>>>>>>>>>>> Sorry I'm responding here without seeing latest 
>>>>>>>>>>>>>>>>>>>>> webrev but there is enough context I think ...
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> On 3/07/2020 9:14 am, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>> Hi Dan,
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Thanks for your comment!
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> On 2020/07/03 7:16, Daniel D. Daugherty wrote:
>>>>>>>>>>>>>>>>>>>>>>> On 7/2/20 5:19 AM, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> I upload new webrev. Could you review again?
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.03/ 
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnv.cpp
>>>>>>>>>>>>>>>>>>>>>>>      L1542:     // Get stack trace with handshake
>>>>>>>>>>>>>>>>>>>>>>>          nit - please add a period at the end.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I will fix it.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> L1591: *stack_info_ptr = op.stack_info();
>>>>>>>>>>>>>>>>>>>>>>>          The return parameter should not be 
>>>>>>>>>>>>>>>>>>>>>>> touched unless the return
>>>>>>>>>>>>>>>>>>>>>>>          code in 'err' == JVMTI_ERROR_NONE.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>      old L1582:   if (err == JVMTI_ERROR_NONE) {
>>>>>>>>>>>>>>>>>>>>>>>          Please restore this check. The return 
>>>>>>>>>>>>>>>>>>>>>>> parameter should not
>>>>>>>>>>>>>>>>>>>>>>>          be touched unless the return code in 
>>>>>>>>>>>>>>>>>>>>>>> 'err' == JVMTI_ERROR_NONE.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I will fix it.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> But op.stack_info() will return NULL if the error 
>>>>>>>>>>>>>>>>>>>>> is not JVMTI_ERROR_NONE. Are you (Dan) concerned 
>>>>>>>>>>>>>>>>>>>>> about someone passing in a non-null/initialized 
>>>>>>>>>>>>>>>>>>>>> out-pointer that will be reset to NULL if there was 
>>>>>>>>>>>>>>>>>>>>> an error?
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Actually the way we used to test this in POSIX tests 
>>>>>>>>>>>>>>>>>>>> is to call
>>>>>>>>>>>>>>>>>>>> an API with known bad parameters and the return 
>>>>>>>>>>>>>>>>>>>> parameter ptr
>>>>>>>>>>>>>>>>>>>> set to NULL. If the return parameter ptr was touched 
>>>>>>>>>>>>>>>>>>>> when an
>>>>>>>>>>>>>>>>>>>> error should have been detected on an earlier 
>>>>>>>>>>>>>>>>>>>> parameter, then
>>>>>>>>>>>>>>>>>>>> the test failed.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> L1272:   if (!jt->is_exiting() && (thread_oop != 
>>>>>>>>>>>>>>>>>>>>>>> NULL)) {
>>>>>>>>>>>>>>>>>>>>>>>          nit - extra parens around the second 
>>>>>>>>>>>>>>>>>>>>>>> expression.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I will fix it.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>>>>>>>>>>>>>>>>>>>>>>>      old L1532:   _result = 
>>>>>>>>>>>>>>>>>>>>>>> JVMTI_ERROR_THREAD_NOT_ALIVE;
>>>>>>>>>>>>>>>>>>>>>>>          This deletion of the _result field threw 
>>>>>>>>>>>>>>>>>>>>>>> me for a minute and then
>>>>>>>>>>>>>>>>>>>>>>>          I figured out that the field is init to 
>>>>>>>>>>>>>>>>>>>>>>> JVMTI_ERROR_THREAD_NOT_ALIVE
>>>>>>>>>>>>>>>>>>>>>>>          in the constructor.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>      L1553:   if (!jt->is_exiting() && 
>>>>>>>>>>>>>>>>>>>>>>> (jt->threadObj() != NULL)) {
>>>>>>>>>>>>>>>>>>>>>>>          nit - extra parens around the second 
>>>>>>>>>>>>>>>>>>>>>>> expression.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I will fix it.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvBase.hpp
>>>>>>>>>>>>>>>>>>>>>>>      No comments.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/runtime/vmOperations.hpp
>>>>>>>>>>>>>>>>>>>>>>>      No comments.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/GetThreadListStackTraces.java 
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>      No comments.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/OneGetThreadListStackTraces.java 
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>      L64: startSignal.countDown();
>>>>>>>>>>>>>>>>>>>>>>>          I was expecting this to be a call to 
>>>>>>>>>>>>>>>>>>>>>>> await() instead of
>>>>>>>>>>>>>>>>>>>>>>>          countDown(). What am I missing here?
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>          I think this test might be passing by 
>>>>>>>>>>>>>>>>>>>>>>> accident right now, but...
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Main thread (which call JVMTI functions to test) 
>>>>>>>>>>>>>>>>>>>>>> should wait until test thread is ready.
>>>>>>>>>>>>>>>>>>>>>> So main thread would wait startSignal, and test 
>>>>>>>>>>>>>>>>>>>>>> thread would count down.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> No!
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> The test thread that previously called obj.wait() 
>>>>>>>>>>>>>>>>>>>>> now calls latch.await().
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> The main thread that previously called obj.notify() 
>>>>>>>>>>>>>>>>>>>>> now calls latch.countDown().
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> The main thread continues to spin until it sees the 
>>>>>>>>>>>>>>>>>>>>> target is WAITING before proceeding with the test.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> If I add spin wait to wait until transit target 
>>>>>>>>>>>>>>>>>>> thread state is WAITING (as following), we don't need 
>>>>>>>>>>>>>>>>>>> to call SuspendThread().
>>>>>>>>>>>>>>>>>>> Which is better?
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> The original spin-wait loop checking for  WAITING is 
>>>>>>>>>>>>>>>>>> better because it is the only guarantee that the 
>>>>>>>>>>>>>>>>>> target thread is blocked where you need it to be. 
>>>>>>>>>>>>>>>>>> suspending the thread is racy as you don't know 
>>>>>>>>>>>>>>>>>> exactly where the suspend will hit.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>> /* Wait until the thread state transits to "waiting" */
>>>>>>>>>>>>>>>>>>> while (th.getState() != Thread.State.WAITING) {
>>>>>>>>>>>>>>>>>>>      Thread.onSpinWait();
>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> For simplify, spin wait is prefer to 
>>>>>>>>>>>>>>>>>>> OneGetThreadListStackTraces.java in webrev.03.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Here's the flow as I see it:
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> main thread
>>>>>>>>>>>>>>>>>>>>    - start worker thread
>>>>>>>>>>>>>>>>>>>>    - startSignal.await()
>>>>>>>>>>>>>>>>>>>>      - main is now blocked
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> worker thread
>>>>>>>>>>>>>>>>>>>>    - startSignal.countDown()
>>>>>>>>>>>>>>>>>>>>      - main is now unblocked
>>>>>>>>>>>>>>>>>>>>    - stopSignal.await()
>>>>>>>>>>>>>>>>>>>>      - worker is now blocked
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> main thread
>>>>>>>>>>>>>>>>>>>>    - checkCallStacks(th)
>>>>>>>>>>>>>>>>>>>>    - stopSignal.countDown()
>>>>>>>>>>>>>>>>>>>>      - worker is now unblocked
>>>>>>>>>>>>>>>>>>>>    - th.join
>>>>>>>>>>>>>>>>>>>>      - main is now blocked
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> worker thread
>>>>>>>>>>>>>>>>>>>>    - runs off the end of run()
>>>>>>>>>>>>>>>>>>>>      - main is now unblocked
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> main thread
>>>>>>>>>>>>>>>>>>>>    - run off the end of main()
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libGetThreadListStackTraces.c 
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>      L92:   jthreads = (jthread 
>>>>>>>>>>>>>>>>>>>>>>> *)malloc(sizeof(jthread) * num_threads);
>>>>>>>>>>>>>>>>>>>>>>>          You don't check for malloc() failure.
>>>>>>>>>>>>>>>>>>>>>>>          'jthreads' is allocated but never freed.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I will fix it.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.c 
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>      L91:   result = 
>>>>>>>>>>>>>>>>>>>>>>> (*jvmti)->SuspendThread(jvmti, thread);
>>>>>>>>>>>>>>>>>>>>>>>          Why are you suspending the thread? 
>>>>>>>>>>>>>>>>>>>>>>> GetAllStackTraces() and
>>>>>>>>>>>>>>>>>>>>>>> GetThreadListStackTraces() do not require the 
>>>>>>>>>>>>>>>>>>>>>>> target thread(s)
>>>>>>>>>>>>>>>>>>>>>>>          to be suspend.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>          If you decide not to SuspendThread, then 
>>>>>>>>>>>>>>>>>>>>>>> you don't need the
>>>>>>>>>>>>>>>>>>>>>>>          AddCapabilities or the ResumeThread calls.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Test thread might not be entered following code 
>>>>>>>>>>>>>>>>>>>>>> (stopSignal.await()). We might see deferent call 
>>>>>>>>>>>>>>>>>>>>>> stack between GetAllStackTraces() and 
>>>>>>>>>>>>>>>>>>>>>> GetThreadListStackTraces(). We cannot control to 
>>>>>>>>>>>>>>>>>>>>>> freeze call stack of test thread in Java code.
>>>>>>>>>>>>>>>>>>>>>> (I didn't use SuspendThread() at first, but I saw 
>>>>>>>>>>>>>>>>>>>>>> some errors which causes in above.)
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> So we need to call SuspendThread() to ensure we 
>>>>>>>>>>>>>>>>>>>>>> can see same call stack.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> If you are checking that the thread is in state 
>>>>>>>>>>>>>>>>>>>>> WAITING then it cannot escape from that state and 
>>>>>>>>>>>>>>>>>>>>> you can sample the stack multiple times from any 
>>>>>>>>>>>>>>>>>>>>> API and get the same result.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> I suspect the errors you saw were from the apparent 
>>>>>>>>>>>>>>>>>>>>> incorrect use of the CountDownLatch.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> With the flow outlined above, the worker thread 
>>>>>>>>>>>>>>>>>>>> should be
>>>>>>>>>>>>>>>>>>>> nicely blocked in stopSignal.await() when stuff is 
>>>>>>>>>>>>>>>>>>>> sampled.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Dan
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Dan
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> On 2020/07/02 15:05, David Holmes wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> On 1/07/2020 11:53 am, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> I uploaded new webrev. Could review again?
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.02/ 
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Updates look fine - thanks.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> One minor nit:
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> 1274 _collector.allocate_and_fill_stacks(1);
>>>>>>>>>>>>>>>>>>>>>>>>> 1275 _collector.set_result(JVMTI_ERROR_NONE);
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> In the other places where you use _collector 
>>>>>>>>>>>>>>>>>>>>>>>>> you rely on result being initialized to 
>>>>>>>>>>>>>>>>>>>>>>>>> JVMTI_ERROR_NONE, rather than setting it 
>>>>>>>>>>>>>>>>>>>>>>>>> directly after allocate_and_fill_stacks().
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Fixed.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   820 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> assert(SafepointSynchronize::is_at_safepoint() 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ||
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   821 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> java_thread->is_thread_fully_suspended(false, 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> &debug_bits) ||
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   822 current_thread == 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> java_thread->active_handshaker(),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   823 "at safepoint / handshake or target 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread is suspended");
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I don't think the suspension check is 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> necessary, as even if the target is 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> suspended we must still be at a safepoint 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> or in a handshake with it. Makes me 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wonder if we used to allow a racy 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> stacktrace operation on a suspended 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread, assuming it would remain suspended?
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> This function 
>>>>>>>>>>>>>>>>>>>>>>>>>> (JvmtiEnvBase::get_stack_trace()) can be 
>>>>>>>>>>>>>>>>>>>>>>>>>> called to get own stack trace. For example, we 
>>>>>>>>>>>>>>>>>>>>>>>>>> can call GetStackTrace() for current thread at 
>>>>>>>>>>>>>>>>>>>>>>>>>> JVMTI event.
>>>>>>>>>>>>>>>>>>>>>>>>>> So I changed assert as below:
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>   820 assert(current_thread == java_thread ||
>>>>>>>>>>>>>>>>>>>>>>>>>>   821 SafepointSynchronize::is_at_safepoint() ||
>>>>>>>>>>>>>>>>>>>>>>>>>>   822 current_thread == 
>>>>>>>>>>>>>>>>>>>>>>>>>> java_thread->active_handshaker(),
>>>>>>>>>>>>>>>>>>>>>>>>>>   823          "call by myself / at safepoint 
>>>>>>>>>>>>>>>>>>>>>>>>>> / at handshake");
>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Yep good catch. I hope current tests caught that.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> They would be tested in 
>>>>>>>>>>>>>>>>>>>>>>>> vmTestbase/nsk/jvmti/GetStackTrace/getstacktr001/ (own 
>>>>>>>>>>>>>>>>>>>>>>>> call stacks), and getstacktr003 (call stacks in 
>>>>>>>>>>>>>>>>>>>>>>>> other thread).
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Speaking of tests ...
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> In the native code I think you need to check 
>>>>>>>>>>>>>>>>>>>>>>>>> the success of all JNI methods that can throw 
>>>>>>>>>>>>>>>>>>>>>>>>> exceptions - otherwise I believe the tests may 
>>>>>>>>>>>>>>>>>>>>>>>>> trigger warnings if -Xcheck:jni is used with 
>>>>>>>>>>>>>>>>>>>>>>>>> them. See for example:
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.cpp 
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> I updated testcases to check JNI and JVMTI 
>>>>>>>>>>>>>>>>>>>>>>>> function calls.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> In the Java code the target thread:
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>    45     public void run() {
>>>>>>>>>>>>>>>>>>>>>>>>>    46       try {
>>>>>>>>>>>>>>>>>>>>>>>>>    47 synchronized (lock) {
>>>>>>>>>>>>>>>>>>>>>>>>>    48 lock.wait();
>>>>>>>>>>>>>>>>>>>>>>>>>    49 System.out.println("OK");
>>>>>>>>>>>>>>>>>>>>>>>>>    50           }
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> is potentially susceptible to spurious wakeups. 
>>>>>>>>>>>>>>>>>>>>>>>>> Using a CountDownLatch would be robust.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Fixed.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> On 2020/07/01 8:48, David Holmes wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> On 1/07/2020 9:05 am, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1271 ResourceMark rm;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> IIUC at this point the _calling_thread is 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the current thread, so we can use:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ResourceMark rm(_calling_thread);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> If so, we can call make_local() in L1272 
>>>>>>>>>>>>>>>>>>>>>>>>>>>> without JavaThread (or we can pass current 
>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread to make_local()). Is it right?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1271 ResourceMark rm;
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1272 
>>>>>>>>>>>>>>>>>>>>>>>>>>>> _collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread, 
>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread_oop),
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1273 jt, thread_oop);
>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Sorry I got confused, _calling_thread may not 
>>>>>>>>>>>>>>>>>>>>>>>>>>> be the current thread as we could be 
>>>>>>>>>>>>>>>>>>>>>>>>>>> executing the handshake in the target thread 
>>>>>>>>>>>>>>>>>>>>>>>>>>> itself. So the ResourceMark is correct as-is 
>>>>>>>>>>>>>>>>>>>>>>>>>>> (implicitly for current thread).
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> The argument to fill_frames will be used in 
>>>>>>>>>>>>>>>>>>>>>>>>>>> the jvmtiStackInfo and passed back to the 
>>>>>>>>>>>>>>>>>>>>>>>>>>> _calling_thread, so it must be created via 
>>>>>>>>>>>>>>>>>>>>>>>>>>> make_local(_calling_thread, ...) as you 
>>>>>>>>>>>>>>>>>>>>>>>>>>> presently have.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 2020/07/01 7:05, David Holmes wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 1/07/2020 12:17 am, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thank you for reviewing! I will update new 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> webrev tomorrow.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 466 class MultipleStackTracesCollector : 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> public StackObj {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   498 class VM_GetAllStackTraces : public 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> VM_Operation {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   499 private:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   500 JavaThread *_calling_thread;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   501   jint _final_thread_count;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   502 MultipleStackTracesCollector 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _collector;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> You can't have a StackObj as a member of 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> another class like that as it may not be 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> on the stack. I think 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> MultipleStackTracesCollector should not 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> extend any allocation class, and should 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> always be embedded directly in another 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> class.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I'm not sure what does mean "embedded".
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Is it ok as below?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> class MultipleStackTracesCollector {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>     :
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> class GetAllStackTraces : public 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> VM_Operation {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>    private:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> MultipleStackTracesCollector _collector;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yes that I what I meant.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 2020/06/30 22:22, David Holmes wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 30/06/2020 10:05 am, Yasumasa Suenaga 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi David, Serguei,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I updated webrev for 8242428. Could you 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> review again?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This change migrate to use direct 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> handshake for GetStackTrace() and 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GetThreadListStackTraces() (when 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread_count == 1).
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/ 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This looks really good now! I only have a 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> few nits below. There is one thing I 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> don't like about it but it requires a 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> change to the main Handshake logic to 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> address - in 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> JvmtiEnv::GetThreadListStackTraces you 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> have to create a ThreadsListHandle to 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> convert the jthread to a JavaThread, but 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> then the Handshake::execute_direct 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> creates another ThreadsListHandle 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> internally. That's a waste. I will 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> discuss with Robbin and file a RFE to 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> have an overload of execute_direct that 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> takes an existing TLH. Actually it's 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> worse than that because we have another 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> TLH in use at the entry point for the 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> JVMTI functions, so I think there may be 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> some scope for simplifying the use of TLH 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> instances - future RFE.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvBase.hpp
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   451 GetStackTraceClosure(JvmtiEnv *env, 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> jint start_depth, jint max_count,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   452 jvmtiFrameInfo* frame_buffer, jint* 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> count_ptr)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   453     : 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> HandshakeClosure("GetStackTrace"),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   454 _env(env), 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _start_depth(start_depth), 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _max_count(max_count),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   455 _frame_buffer(frame_buffer), 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _count_ptr(count_ptr),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   456 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _result(JVMTI_ERROR_THREAD_NOT_ALIVE) {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Nit: can you do one initializer per line 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> please.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This looks wrong:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 466 class MultipleStackTracesCollector : 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> public StackObj {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   498 class VM_GetAllStackTraces : public 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> VM_Operation {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   499 private:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   500 JavaThread *_calling_thread;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   501   jint _final_thread_count;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   502 MultipleStackTracesCollector 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _collector;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> You can't have a StackObj as a member of 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> another class like that as it may not be 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> on the stack. I think 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> MultipleStackTracesCollector should not 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> extend any allocation class, and should 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> always be embedded directly in another 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> class.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 481 MultipleStackTracesCollector(JvmtiEnv 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> *env, jint max_frame_count) {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   482     _env = env;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   483 _max_frame_count = max_frame_count;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   484 _frame_count_total = 0;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   485 _head = NULL;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   486 _stack_info = NULL;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   487 _result = JVMTI_ERROR_NONE;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   488   }
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> As you are touching this can you change 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it to use an initializer list as you did 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> for the HandshakeClosure, and please keep 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> one item per line.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   820 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> assert(SafepointSynchronize::is_at_safepoint() 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ||
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   821 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> java_thread->is_thread_fully_suspended(false, 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> &debug_bits) ||
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   822 current_thread == 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> java_thread->active_handshaker(),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   823 "at safepoint / handshake or target 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread is suspended");
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I don't think the suspension check is 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> necessary, as even if the target is 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> suspended we must still be at a safepoint 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> or in a handshake with it. Makes me 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wonder if we used to allow a racy 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> stacktrace operation on a suspended 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread, assuming it would remain suspended?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1268   oop thread_oop = jt->threadObj();
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1269
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1270   if (!jt->is_exiting() && 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (jt->threadObj() != NULL)) {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> You can use thread_oop in line 1270.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1272 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread, 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread_oop),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1273 jt, thread_oop);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> It is frustrating that this entire call 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> chain started with a jthread reference, 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> which we converted to a JavaThread, only 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to eventually need to convert it back to 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> a jthread! I think there is some scope 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> for simplification here but not as part 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> of this change.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1271 ResourceMark rm;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> IIUC at this point the _calling_thread is 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the current thread, so we can use:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ResourceMark rm(_calling_thread);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Please add @bug lines to the tests.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I'm still pondering the test logic but 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wanted to send this now.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> VM_GetThreadListStackTrace (for 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GetThreadListStackTraces) and 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> VM_GetAllStackTraces (for 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GetAllStackTraces) have inherited 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> VM_GetMultipleStackTraces VM operation 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> which provides the feature to generate 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> jvmtiStackInfo. I modified 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> VM_GetMultipleStackTraces to a normal 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> C++ class to share with HandshakeClosure 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> for GetThreadListStackTraces 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (GetSingleStackTraceClosure).
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Also I added new testcases which test 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GetThreadListStackTraces() with 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread_count == 1 and with all threads.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This change has been tested in 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> serviceability/jvmti serviceability/jdwp 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> vmTestbase/nsk/jvmti vmTestbase/nsk/jdi 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> vmTestbase/nsk/jdwp.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 2020/06/24 15:50, Yasumasa Suenaga 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Please review this change:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>    JBS: 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8242428 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>    webrev: 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/ 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This change replace following VM 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> operations to direct handshake.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   - VM_GetFrameCount (GetFrameCount())
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   - VM_GetFrameLocation 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (GetFrameLocation())
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   - VM_GetThreadListStackTraces 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (GetThreadListStackTrace())
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>   - VM_GetCurrentLocation
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GetThreadListStackTrace() uses direct 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> handshake if thread count == 1. In 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> other case (thread count > 1), it would 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> be performed as VM operation 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (VM_GetThreadListStackTraces).
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Caller of VM_GetCurrentLocation 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (JvmtiEnvThreadState::reset_current_location()) 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> might be called at safepoint. So I 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> added safepoint check in its caller.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This change has been tested in 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> serviceability/jvmti 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> serviceability/jdwp 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> vmTestbase/nsk/jvmti vmTestbase/nsk/jdi 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> vmTestbase/ns
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> k/jdwp.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Also I tested it on submit repo, then 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it has execution error 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (mach5-one-ysuenaga-JDK-8242428-20200624-0054-12034717) 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> due to dependency error. So I think it 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> does not occur by this change.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>


More information about the serviceability-dev mailing list