RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Sun Jul 5 05:46:53 UTC 2020
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