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.spit...@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.spit...@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
