RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake

[David Holmes]

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