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

Fri, 03 Jul 2020 12:35:23 -0700 

Hi Yasumasa,

This difference is not that big to care about.
I feel this is really rare case and so, does not worth these complications.
Do we have a real request from customers to optimize it?

Thanks,
Serguei


On 7/3/20 01:16, Yasumasa Suenaga wrote:

Hi Serguei,
Generally I agree with you, but I have concern about the difference of the result of GetStackTrace() and GetThreadListStackTraces(). 

  GetStackTrace: jvmtiFrameInfo
  GetThreadListStackTraces: jvmtiStackInfo
jvmtiStackInfo contains thread state, and it is ensured it is the state of the call stack. If we want to get both call stack and thread state, we need to suspend target thread, and call both GetStackTrace() and GetThreadState(). Is it ok?
I was wondering if JDK-8201641 (parent ticket of this change) needed them for profiling (dynatrace?) If it is responsibility of JVMTI agent implementor, I remove this closure. 


Thanks,

Yasumasa


On 2020/07/03 16:45, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

After some thinking I've concluded that I do not like this optimization
of the GetThreadListStackTraces with GetSingleStackTraceClosure.

We may need more opinions on this but these are my points:
  - it adds some complexity and ugliness
  - a win is doubtful because it has to be a rare case, so that total overhead should not be high 
  - if it is really high for some use cases then it is up to the user
    to optimize it with using GetStackTrace instead

In such cases with doubtful overhead I usually prefer the simplicity.
Good examples where it makes sense to optimize are checks for target thread to be current thread. In such cases there is no need to suspend the target thread, or use a VMop/HandshakeClosure. For instance, please, see the Monitor functions with the check: (java_thread == calling_thread). Getting information for current thread is frequently used case, e.g. to get info at an event point. 

Thanks,
Serguei


On 7/2/20 23:29, Yasumasa Suenaga wrote:

Hi Dan, David,

I uploaded new webrev. Could you review again?

http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.04/
OneGetThreadListStackTraces.java in this webrev would wait until thread state is transited to "waiting" with spin wait. 
CountDownLatch::await call as Dan pointed is fixed in it :)

Diff from webrev.03:
http://hg.openjdk.java.net/jdk/submit/rev/c9aeb7001e50


Thanks,

Yasumasa


On 2020/07/03 14:15, David Holmes wrote:



On 3/07/2020 2:27 pm, Yasumasa Suenaga wrote:

On 2020/07/03 12:24, Daniel D. Daugherty wrote:

On 7/2/20 10:50 PM, David Holmes wrote:
Sorry I'm responding here without seeing latest webrev but there is enough context I think ... 

On 3/07/2020 9:14 am, Yasumasa Suenaga wrote:

Hi Dan,

Thanks for your comment!

On 2020/07/03 7:16, Daniel D. Daugherty wrote:

On 7/2/20 5:19 AM, Yasumasa Suenaga wrote:

Hi David,

I upload new webrev. Could you review again?

http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.03/


src/hotspot/share/prims/jvmtiEnv.cpp
     L1542:     // Get stack trace with handshake
         nit - please add a period at the end.


I will fix it.



     L1591: *stack_info_ptr = op.stack_info();
         The return parameter should not be touched unless the return 
         code in 'err' == JVMTI_ERROR_NONE.

     old L1582:   if (err == JVMTI_ERROR_NONE) {
         Please restore this check. The return parameter should not          be touched unless the return code in 'err' == JVMTI_ERROR_NONE. 

I will fix it.
But op.stack_info() will return NULL if the error is not JVMTI_ERROR_NONE. Are you (Dan) concerned about someone passing in a non-null/initialized out-pointer that will be reset to NULL if there was an error? 

Actually the way we used to test this in POSIX tests is to call
an API with known bad parameters and the return parameter ptr
set to NULL. If the return parameter ptr was touched when an
error should have been detected on an earlier parameter, then
the test failed.







     L1272:   if (!jt->is_exiting() && (thread_oop != NULL)) {
         nit - extra parens around the second expression.


I will fix it.



src/hotspot/share/prims/jvmtiEnvBase.cpp
     old L1532:   _result = JVMTI_ERROR_THREAD_NOT_ALIVE;
         This deletion of the _result field threw me for a minute and then          I figured out that the field is init to JVMTI_ERROR_THREAD_NOT_ALIVE 
         in the constructor.
     L1553:   if (!jt->is_exiting() && (jt->threadObj() != NULL)) { 
         nit - extra parens around the second expression.


I will fix it.



src/hotspot/share/prims/jvmtiEnvBase.hpp
     No comments.

src/hotspot/share/runtime/vmOperations.hpp
     No comments.
test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/GetThreadListStackTraces.java 
     No comments.
test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/OneGetThreadListStackTraces.java 
     L64:         startSignal.countDown();
         I was expecting this to be a call to await() instead of
         countDown(). What am I missing here?
         I think this test might be passing by accident right now, but...
Main thread (which call JVMTI functions to test) should wait until test thread is ready. So main thread would wait startSignal, and test thread would count down. 

No!
The test thread that previously called obj.wait() now calls latch.await().
The main thread that previously called obj.notify() now calls latch.countDown().
The main thread continues to spin until it sees the target is WAITING before proceeding with the test.
If I add spin wait to wait until transit target thread state is WAITING (as following), we don't need to call SuspendThread(). 
Which is better?
The original spin-wait loop checking for  WAITING is better because it is the only guarantee that the target thread is blocked where you need it to be. suspending the thread is racy as you don't know exactly where the suspend will hit. 

Thanks,
David
-----



```
/* Wait until the thread state transits to "waiting" */
while (th.getState() != Thread.State.WAITING) {
     Thread.onSpinWait();
}
```
For simplify, spin wait is prefer to OneGetThreadListStackTraces.java in webrev.03. 


Thanks,

Yasumasa



Here's the flow as I see it:

main thread
   - start worker thread
   - startSignal.await()
     - main is now blocked

worker thread
   - startSignal.countDown()
     - main is now unblocked
   - stopSignal.await()
     - worker is now blocked

main thread
   - checkCallStacks(th)
   - stopSignal.countDown()
     - worker is now unblocked
   - th.join
     - main is now blocked

worker thread
   - runs off the end of run()
     - main is now unblocked

main thread
   - run off the end of main()
test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libGetThreadListStackTraces.c      L92:   jthreads = (jthread *)malloc(sizeof(jthread) * num_threads); 
         You don't check for malloc() failure.
         'jthreads' is allocated but never freed.


I will fix it.
test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.c 
     L91:   result = (*jvmti)->SuspendThread(jvmti, thread);
         Why are you suspending the thread? GetAllStackTraces() and          GetThreadListStackTraces() do not require the target thread(s) 
         to be suspend.
         If you decide not to SuspendThread, then you don't need the 
         AddCapabilities or the ResumeThread calls.
Test thread might not be entered following code (stopSignal.await()). We might see deferent call stack between GetAllStackTraces() and GetThreadListStackTraces(). We cannot control to freeze call stack of test thread in Java code. (I didn't use SuspendThread() at first, but I saw some errors which causes in above.)
So we need to call SuspendThread() to ensure we can see same call stack.
If you are checking that the thread is in state WAITING then it cannot escape from that state and you can sample the stack multiple times from any API and get the same result.
I suspect the errors you saw were from the apparent incorrect use of the CountDownLatch. 

With the flow outlined above, the worker thread should be
nicely blocked in stopSignal.await() when stuff is sampled.

Dan




Cheers,
David
-----



Thanks,

Yasumasa



Dan



On 2020/07/02 15:05, David Holmes wrote:

Hi Yasumasa,

On 1/07/2020 11:53 am, Yasumasa Suenaga wrote:

Hi,

I uploaded new webrev. Could review again?

http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.02/


Updates look fine - thanks.

One minor nit:

1274 _collector.allocate_and_fill_stacks(1);
1275 _collector.set_result(JVMTI_ERROR_NONE);
In the other places where you use _collector you rely on result being initialized to JVMTI_ERROR_NONE, rather than setting it directly after allocate_and_fill_stacks(). 

Fixed.



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?
This function (JvmtiEnvBase::get_stack_trace()) can be called to get own stack trace. For example, we can call GetStackTrace() for current thread at JVMTI event. 
So I changed assert as below:

```
  820   assert(current_thread == java_thread ||
  821 SafepointSynchronize::is_at_safepoint() ||
  822          current_thread == java_thread->active_handshaker(),   823          "call by myself / at safepoint / at handshake"); 
```


Yep good catch. I hope current tests caught that.
They would be tested in vmTestbase/nsk/jvmti/GetStackTrace/getstacktr001/ (own call stacks), and getstacktr003 (call stacks in other thread). 



Speaking of tests ...
In the native code I think you need to check the success of all JNI methods that can throw exceptions - otherwise I believe the tests may trigger warnings if -Xcheck:jni is used with them. See for example:
test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.cpp 

I updated testcases to check JNI and JVMTI function calls.



In the Java code the target thread:

   45     public void run() {
   46       try {
   47           synchronized (lock) {
   48               lock.wait();
   49               System.out.println("OK");
   50           }
is potentially susceptible to spurious wakeups. Using a CountDownLatch would be robust. 

Fixed.


Thanks,

Yasumasa



Thanks,
David
-----



Thanks,

Yasumasa


On 2020/07/01 8:48, David Holmes wrote:

Hi Yasumasa,

On 1/07/2020 9:05 am, Yasumasa Suenaga wrote:

Hi David,


1271 ResourceMark rm;
IIUC at this point the _calling_thread is the current thread, so we can use: 

     ResourceMark rm(_calling_thread);
If so, we can call make_local() in L1272 without JavaThread (or we can pass current thread to make_local()). Is it right? 

```
1271     ResourceMark rm;
1272 _collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread, thread_oop), 
1273                            jt, thread_oop);
```
Sorry I got confused, _calling_thread may not be the current thread as we could be executing the handshake in the target thread itself. So the ResourceMark is correct as-is (implicitly for current thread).
The argument to fill_frames will be used in the jvmtiStackInfo and passed back to the _calling_thread, so it must be created via make_local(_calling_thread, ...) as you presently have. 

Thanks,
David


Thanks,

Yasumasa


On 2020/07/01 7:05, David Holmes wrote:

On 1/07/2020 12:17 am, Yasumasa Suenaga wrote:

Hi David,
Thank you for reviewing! I will update new webrev tomorrow.
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. 

I'm not sure what does mean "embedded".
Is it ok as below?

```
class MultipleStackTracesCollector {
    :
}

class GetAllStackTraces : public VM_Operation {
   private:
     MultipleStackTracesCollector _collector;
}
```


Yes that I what I meant.

Thanks,
David
-----



Thanks,

Yasumasa


On 2020/06/30 22:22, David Holmes wrote:

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