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

David Holmes david.holmes at oracle.com
Wed Jul 8 06:27:51 UTC 2020


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'm going to be away for the next couple of days - sorry - but will try 
to check email on this if I can.

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