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

Reply via email to