> 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