Hi Serguei,
Thanks for your comment!
I will fix them after JDK-8248379.
Yasumasa
On 2020/06/26 13:48, serguei.spit...@oracle.com wrote:
Hi Yasumasa,
I agree with the approach to separate this into different bugs.
At least, it would be nice to separate the stack trace functions.
It will help to better focus on each fix and improve review quality.
I'd wait for new webrevs from you before diving deep with reviewing.
A couple of quick comments.
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/src/hotspot/share/prims/jvmtiEnv.cpp.udiff.html
I also do not like the complexity of the stack trace updateand extra boolean
argument in GetStackTraceClosure.
It feels like it can be simpler.
I'd suggest to do some renaming as the identifiers you use are not typical in
the jvmtiEnv.cpp:
target_javathread => java_thread
actual_frame_count => frame_count
The GetStackTraceClosureis better to have the same stack_info() function as
VM_op:
*stack_info_ptr = op.stack_info();
At least, this part will be unified between VM_op and HandshakeClosure.
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/src/hotspot/share/prims/jvmtiThreadState.cpp.udiff.html
- (JavaThread *)Thread::current() == get_thread(),
- "must be current thread or at safepoint");
+ current_thread == get_thread() ||
+ (current_thread->is_Java_thread() && (current_thread ==
get_thread()->active_handshaker())),
+ "must be at safepoint or target thread is suspended");
There is no check that the target thread is suspended.
You, probably, wanted to say about handshake instead.
Thanks,
Serguei
On 6/23/20 23: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
