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

Reply via email to