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
