On 7/9/20 12:00 PM, Patricio Chilano wrote:
Hi Yasumasa,
On 7/9/20 9:30 AM, Yasumasa Suenaga wrote:
On 2020/07/09 17:58, David Holmes wrote:
Hi Yasumasa,
On 9/07/2020 10:25 am, Yasumasa Suenaga wrote:
Hi Dan,
Thanks for your comment!
I uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.09/
Diff from previous webrev:
http://hg.openjdk.java.net/jdk/submit/rev/5d167adf8524
I saw similar build errors in
libOneGetThreadListStackTraces.cpp on Windows.
This webrev fixes them.
You shouldn't use %p as it may not be portable. In the VM we
use INTPTR_FORMAT and convert the arg using p2i. I don't know
what exists in the testing code.
I replaced %p to %lx, and also cast values to unsigned long [1]
[2], but the test on submit repo was failed.
Can anyone share details of
mach5-one-ysuenaga-JDK-8242428-20200709-1030-12500928 ?
These are the errors I see for the macOS build:
./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:53:14:
error: format specifies type 'long long' but the argument has
type 'jlocation' (aka 'long') [-Werror,-Wformat]
fi1->location, fi2->location);
^~~~~~~~~~~~~
./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:53:29:
error: format specifies type 'long long' but the argument has
type 'jlocation' (aka 'long') [-Werror,-Wformat]
fi1->location, fi2->location);
^~~~~~~~~~~~~
These are the ones I see for the Windows build:
./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(48):
warning C4311: 'type cast': pointer truncation from 'jmethodID'
to 'unsigned long'
./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(48):
warning C4302: 'type cast': truncation from 'jmethodID' to
'unsigned long'
./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(48):
warning C4311: 'type cast': pointer truncation from 'jmethodID'
to 'unsigned long'
./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(48):
warning C4302: 'type cast': truncation from 'jmethodID' to
'unsigned long'
./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(70):
warning C4311: 'type cast': pointer truncation from 'jthread' to
'unsigned long'
./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(70):
warning C4302: 'type cast': truncation from 'jthread' to
'unsigned long'
./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(70):
warning C4311: 'type cast': pointer truncation from 'jthread' to
'unsigned long'
./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(70):
warning C4302: 'type cast': truncation from 'jthread' to
'unsigned long'
You will probably want to use the macros defined in
src/hotspot/share/utilities/globalDefinitions.hpp. Let me know
if you need me to test something.
With these changes the build works okay on Linux, Windows and macOS:
---
a/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
+++
b/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
@@ -27,4 +27,5 @@
#include <stdio.h>
#include <stdlib.h>
+#include <inttypes.h>
#define MAX_FRAMES 100
@@ -45,11 +46,11 @@
if (fi1->method != fi2->method) { /* jvmtiFrameInfo::method */
snprintf(err_msg, sizeof(err_msg),
- "method is different: fi1 = %p, fi2 = %p",
- fi1->method, fi2->method);
+ "method is different: fi1 = 0x%016" PRIxPTR " , fi2
= 0x%016" PRIxPTR,
+ (intptr_t)fi1->method, (intptr_t)fi2->method);
env->FatalError(err_msg);
} else if (fi1->location != fi2->location) { /*
jvmtiFrameInfo::location */
snprintf(err_msg, sizeof(err_msg),
- "location is different: fi1 = %lld, fi2 = %lld",
- fi1->location, fi2->location);
+ "location is different: fi1 = %" PRId64 " , fi2 =
%" PRId64,
+ (int64_t)fi1->location, (int64_t)fi2->location);
env->FatalError(err_msg);
}
@@ -67,5 +68,5 @@
if (!is_same) { /* jvmtiStackInfo::thread */
snprintf(err_msg, sizeof(err_msg),
- "thread is different: si1 = %p, si2 = %p",
si1->thread, si2->thread);
+ "thread is different: si1 = 0x%016" PRIxPTR " , si2
= 0x%016" PRIxPTR, (intptr_t)si1->thread, (intptr_t)si2->thread);
env->FatalError(err_msg);
} else if (si1->state != si2->state) { /*
jvmtiStackInfo::state */
Maybe you can use something like that.
Thanks,
Patricio
Thanks,
Patricio
Thanks,
Yasumasa
[1] http://hg.openjdk.java.net/jdk/submit/rev/dfca51958217
[2] http://hg.openjdk.java.net/jdk/submit/rev/3665361fa91b
David
-----
Thanks,
Yasumasa
On 2020/07/09 1:42, Daniel D. Daugherty wrote:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.08/
src/hotspot/share/prims/jvmtiEnv.cpp
No comments.
src/hotspot/share/prims/jvmtiEnvBase.cpp
L1159: Thread *current_thread = Thread::current();
Please add "#ifdef ASSERT" above and "#endif" below
since
current_thread is only used for the assert() in
this function.
src/hotspot/share/prims/jvmtiEnvBase.hpp
L549: jthread java_thread, jint max_frame_count)
L552: _jthread(java_thread),
Please: s/java_thread/thread/ on both lines.
src/hotspot/share/runtime/vmOperations.hpp
No comments.
test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/OneGetThreadListStackTraces.java
No comments.
test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
L27: #include <errno.h>
This include is out of order; should be first in
the list.
This file doesn't compile on my MBP13:
./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:49:14:
error: format specifies type 'unsigned long' but the
argument has type 'jmethodID' (aka '_jmethodID *')
[-Werror,-Wformat]
fi1->method, fi2->method);
^~~~~~~~~~~
./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:49:27:
error: format specifies type 'unsigned long' but the
argument has type 'jmethodID' (aka '_jmethodID *')
[-Werror,-Wformat]
fi1->method, fi2->method);
^~~~~~~~~~~
2 errors generated.
This change made it compile on my MBP13, but that may
break it on
other platforms:
$ hg diff
diff -r 560847c69fbe
test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
---
a/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
Wed Jul 08 12:13:32 2020 -0400
+++
b/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
Wed Jul 08 12:40:42 2020 -0400
@@ -46,7 +46,7 @@
if (fi1->method != fi2->method) { /*
jvmtiFrameInfo::method */
snprintf(err_msg, sizeof(err_msg),
"method is different: fi1 = %lx, fi2 = %lx",
- fi1->method, fi2->method);
+ (unsigned long) fi1->method, (unsigned
long) fi2->method);
env->FatalError(err_msg);
} else if (fi1->location != fi2->location) { /*
jvmtiFrameInfo::location */
snprintf(err_msg, sizeof(err_msg),
I'm not sure of the right platform independent way to
output
the 'method' field.
Dan
On 7/8/20 4:04 AM, Yasumasa Suenaga wrote:
Hi David,
On 2020/07/08 15:27, David Holmes wrote:
Hi Yasumasa,
On 7/07/2020 6:54 pm, Yasumasa Suenaga wrote:
Hi David, Serguei,
Serguei, thank you for replying even though you are on
vacaiton!
I uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.07/
Diff from previous webrev:
http://hg.openjdk.java.net/jdk/submit/rev/77243b1dcbfe
c'tor of GetSingleStackTraceClosure has jthread argument
in this webrev.
Also it does not contain testcase for
GetThreadListStackTraces with all threads, and
OneGetThreadListStackTraces would test main thread only.
All those changes are fine in principle for me. One
nit/suggestion:
src/hotspot/share/prims/jvmtiEnvBase.hpp
544 jthread _java_thread;
elsewhere "java_thread" refers to a JavaThread, so to
avoid confusion may I suggest this member be named _jthread.
I uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.08/
Diff from previous webrev:
http://hg.openjdk.java.net/jdk/submit/rev/ca6263dbdc87
I'm going to be away for the next couple of days - sorry -
but will try to check email on this if I can.
Thanks!
Yasumasa
Thanks,
David
-----
Thanks,
Yasumasa
On 2020/07/07 15:13, David Holmes wrote:
On 7/07/2020 2:57 pm, Yasumasa Suenaga wrote:
Hi David,
On 2020/07/07 11:31, David Holmes wrote:
Hi Yasumasa,
Hard to keep up with the changes - especially without
incremental webrevs.
Sorry, I will upload diff from previous webrev in the
next.
If GetSingleStackTraceClosure also took the jthread as
a constructor arg, then you wouldn't need to recreate
a JNI local handle when calling
_collector.fill_frames. It's a small simplification
and not essential at this stage.
I think we should get jthread from an argument of
do_thread() because do_thread() would pass the thread
which are stopped certainly.
It might be simplification if we pass _calling_thread
to MultipleStackTracesCollector. `jthread` is only
needed to store jvmtiStackInfo.thread . What do you think?
I'm not quite sure what you mean.
I think there is a bit of a design wart with direct
handshakes in that do_thread takes the target JavaThread
as an argument. That's useful in a case where you want a
HandshakeClosure that can be applied to multiple
threads, but that's not typically what is needed with
direct handshakes - there is only a single target. With
a single-target HandshakeClosure you can capture all the
"target" information for the operation in the closure
instance. So if the actual do_thread operation wants the
jthread corresponding to the target thread then we can
store that in the closure rather than recomputing it
(you could assert it is the same but that seems overkill
to me).
For the test ... I don't see how
Java_GetThreadListStackTraces_checkCallStacks is a
valid test. It gets the stacks of all live threads,
then uses that information to use
GetThreadListStackTraces to get the stack for the same
set of threads through a different API. It then
compares the two sets of stacks for each thread
expecting them to be the same, but that need only be
the case for the main thread. Other threads could
potentially have a different stack (e.g. if this test
is run with JFR enabled there will be additional
threads found.) Further I would have expected that
there already exist tests that check that, for a given
thread (which may be suspended or known to be blocked)
the same stack is found through the two different APIs.
vmTestbase/nsk/jvmti/unit/GetAllStackTraces/getallstktr001
would check all of threads via
GetThreadListStackTraces() and GetAllStackTraces(), so
we might be able to remove
GetThreadListStackTraces.java from this webrev.
Yes. The existing test only examines a set of test
threads that are all blocked on a raw monitor. You do
not need to duplicate that test.
OTOH we don't have testcase for
GetThreadListStackTraces() with thread_count == 1, so
we need to testcase for it (it is
OneGetThreadListStackTraces.java) It would check
whether the state of target thread is "waiting" before
JNI call to call GetThreadListStackTraces(),
Yes we need to test the special cases introduced by your
changes - totally agree - and
OneGetThreadListStackTraces.java is a good test for that.
and also I expect it would not be run with JFR. (it is
not described @run)
The arguments to run with JFR (or a bunch of other
things) can be passed to the jtreg test harness to be
applied to all tests.
Of course we can check GetThreadListStackTraces() with
main thread, but it is not the test for direct
handshake for other thread.
Right - that test already exists as per the above.
Thanks,
David
Thanks,
Yasumasa
Thanks,
David
-----
On 6/07/2020 11:29 pm, Yasumasa Suenaga wrote:
Hi Serguei,
Thanks for your comment!
I think C++ is more simple to implement the test
agent as you said.
So I implement it in C++ in new webrev. Could you
review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.06/
Also I refactored libGetThreadListStackTraces.cpp,
and I've kept exception check after IsSameObject().
Thanks,
Yasumasa
On 2020/07/06 16:32, serguei.spit...@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.spit...@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.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
