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

[David Holmes]

Hi Yasumasa,

On 7/07/2020 6:54 pm, Yasumasa Suenaga wrote:
Hi David, Serguei,
Serguei, thank you for replying even though you are on vacaiton!

I uploaded new webrev:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.07/
   Diff from previous webrev: 
http://hg.openjdk.java.net/jdk/submit/rev/77243b1dcbfe

c'tor of GetSingleStackTraceClosure has jthread argument in this webrev.
Also it does not contain testcase for GetThreadListStackTraces with all 
threads, and OneGetThreadListStackTraces would test main thread only.

All those changes are fine in principle for me. One nit/suggestion:

src/hotspot/share/prims/jvmtiEnvBase.hpp

 544   jthread _java_thread;

elsewhere "java_thread" refers to a JavaThread, so to avoid confusion 
may I suggest this member be named _jthread.

I'm going to be away for the next couple of days - sorry - but will try 
to check email on this if I can.

Thanks,
David
-----


Thanks,

Yasumasa


On 2020/07/07 15:13, David Holmes wrote:
On 7/07/2020 2:57 pm, Yasumasa Suenaga wrote:
Hi David,

On 2020/07/07 11:31, David Holmes wrote:
Hi Yasumasa,

Hard to keep up with the changes - especially without incremental 
webrevs.

Sorry, I will upload diff from previous webrev in the next.


If GetSingleStackTraceClosure also took the jthread as a constructor 
arg, then you wouldn't need to recreate a JNI local handle when 
calling _collector.fill_frames. It's a small simplification and not 
essential at this stage.

I think we should get jthread from an argument of do_thread() because 
do_thread() would pass the thread which are stopped certainly.
It might be simplification if we pass _calling_thread to 
MultipleStackTracesCollector. `jthread` is only needed to store 
jvmtiStackInfo.thread . What do you think?

I'm not quite sure what you mean.

I think there is a bit of a design wart with direct handshakes in that 
do_thread takes the target JavaThread as an argument. That's useful in 
a case where you want a HandshakeClosure that can be applied to 
multiple threads, but that's not typically what is needed with direct 
handshakes - there is only a single target. With a single-target 
HandshakeClosure you can capture all the "target" information for the 
operation in the closure instance. So if the actual do_thread 
operation wants the jthread corresponding to the target thread then we 
can store that in the closure rather than recomputing it (you could 
assert it is the same but that seems overkill to me).


For the test ... I don't see how 
Java_GetThreadListStackTraces_checkCallStacks is a valid test. It 
gets the stacks of all live threads, then uses that information to 
use GetThreadListStackTraces to get the stack for the same set of 
threads through a different API. It then compares the two sets of 
stacks for each thread expecting them to be the same, but that need 
only be the case for the main thread. Other threads could 
potentially have a different stack (e.g. if this test is run with 
JFR enabled there will be additional threads found.) Further I would 
have expected that there already exist tests that check that, for a 
given thread (which may be suspended or known to be blocked) the 
same stack is found through the two different APIs.

vmTestbase/nsk/jvmti/unit/GetAllStackTraces/getallstktr001 would 
check all of threads via GetThreadListStackTraces() and 
GetAllStackTraces(), so we might be able to remove 
GetThreadListStackTraces.java from this webrev.

Yes. The existing test only examines a set of test threads that are 
all blocked on a raw monitor. You do not need to duplicate that test.

OTOH we don't have testcase for GetThreadListStackTraces() with 
thread_count == 1, so we need to testcase for it (it is 
OneGetThreadListStackTraces.java) It would check whether the state of 
target thread is "waiting" before JNI call to call 
GetThreadListStackTraces(),

Yes we need to test the special cases introduced by your changes - 
totally agree - and OneGetThreadListStackTraces.java is a good test 
for that.

and also I expect it would not be run with JFR. (it is not described 
@run)

The arguments to run with JFR (or a bunch of other things) can be 
passed to the jtreg test harness to be applied to all tests.

Of course we can check GetThreadListStackTraces() with main thread, 
but it is not the test for direct handshake for other thread.

Right - that test already exists as per the above.

Thanks,
David


Thanks,

Yasumasa


Thanks,
David
-----

On 6/07/2020 11:29 pm, Yasumasa Suenaga wrote:
Hi Serguei,

Thanks for your comment!

I think C++ is more simple to implement the test agent as you said.
So I implement it in C++ in new webrev. Could you review again?

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

Also I refactored libGetThreadListStackTraces.cpp, and I've kept 
exception check after IsSameObject().


Thanks,

Yasumasa


On 2020/07/06 16:32, serguei.spit...@oracle.com wrote:
Hi Yasumasa,

Thank you for the update.
I think, a pending exception after IsSameObject needs to be checked.

The checkStackInfo() needs one more refactoring as I've already 
suggested.
The body of the loop at L68-L78 should be converted to a function 
check_frame_info.
The si1->frame_buffer[i] and si2->frame_buffer[i] will be passed 
as fi1 and fi2.
The index can be passed as well.
I'm still suggesting to simplify the local exception_msg to 
something shorter like err_msg or exc_msg.

I'm not sure using fatal is right here:

This fragment looks strange:

  152     if ((*env)->IsSameObject(env, stack_info[i].thread, 
thread)) {
  153       target_info = &stack_info[i];
  154       break;
  155     } else if ((*env)->ExceptionOccurred(env)) {
  156       (*env)->ExceptionDescribe(env);
  157       (*env)->FatalError(env, __FILE__);
  158     }

I expected it to be:

    jboolean same = (*env)->IsSameObject(env, 
stack_info[i].thread, thread);
    if ((*env)->ExceptionOccurred(env)) {
      (*env)->ExceptionDescribe(env);
      (*env)->FatalError(env, __FILE__);
    }
    if (same) {
      target_info = &stack_info[i];
      break;
    }

Would it better to port this agent to C++ to simplify this code 
nicer?

Thanks,
Serguei


On 7/5/20 06:13, Yasumasa Suenaga wrote:
Hi Serguei,

Thanks for your comment!
I refactored testcase. Could you review again?

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

It would check Java exception after IsSameObject() call. Does it 
need?
Any exceptions are not described in JNI document[1], and JNI 
implementation (jni_IsSameObject()) does not seem to throw it.


Thanks,

Yasumasa


[1] 
https://docs.oracle.com/en/java/javase/14/docs/specs/jni/functions.html#issameobject 



On 2020/07/05 14:46, serguei.spit...@oracle.com wrote:
Hi Yasumasa,


Okay, thanks.
Then I'm okay to keep the GetSingleStackTraceClosure.


http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.04/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libGetThreadListStackTraces.c.html 

http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.04/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.c.html 


I'm not sure the function 'is_same_thread() is needed.
Why do not use the JNI IsSameObject instead?

It seems to be a typo at L132 and L137.
You, probably. did not want to print the same information for 
stack_info_1[i].frame_buffer[j].XXX twice.

The code at lines 112-142 is not readable.
I'd suggest to make a couple of refactoring steps.

First step to simplify this a little bit would be with some 
renaming and getting rid of indexes:

   71 char err_msg[EXCEPTION_MSG_LEN] = {0};
  ...
  112   /* Iterate all jvmtiStackInfo to check */
  113   for (i = 0; i < num_threads, *exception_msg != '\0'; i++) {
          jvmtiStackInfo *si1 = stack_info_1[i];
          jvmtiStackInfo *si2 = stack_info_2[i];
  114     if (!IsSameObject(env, si1.thread, si2.thread)) { /* 
jvmtiStackInfo::thread */
  115       snprintf(err_msg, sizeof(err_msg),
  116                "thread[%d] is different: stack_info_1 = 
%p, stack_info_2 = %p",
  117                i, sinfo1.thread, sinfo2.thread);
  118     } else if (si1.state != si2.state) { /* 
jvmtiStackInfo::state */
  119       snprintf(err_msg, sizeof(err_msg),
  120                "state[%d] is different: stack_info_1 = %d, 
stack_info_2 = %d",
  121                i, si1.state, si2.state);
  122     } else if (si1.frame_count != si2.frame_count) { /* 
jvmtiStackInfo::frame_count */
  123       snprintf(err_msg, sizeof(err_msg),
  124                "frame_count[%d] is different: stack_info_1 
= %d, stack_info_2 = %d",
  125                i, si1.frame_count, si2.frame_count);
  126     } else {
  127       /* Iterate all jvmtiFrameInfo to check */
  128       for (j = 0; j < si1.frame_count; j++) {
  129         if (si1.frame_buffer[j].method != 
si1.frame_buffer[j].method) { /* jvmtiFrameInfo::method */
  130           snprintf(err_msg, sizeof(err_msg),
  131                    "thread [%d] frame_buffer[%d].method is 
different: stack_info_1 = %lx, stack_info_2 = %lx",
  132                    i, j, si1.frame_buffer[j].method, 
si2.frame_buffer[j].method);
  133           break;
  134         } else if (si1.frame_buffer[j].location != 
si1.frame_buffer[j].location) { /* jvmtiFrameInfo::location */
  135           snprintf(err_msg, sizeof(err_msg),
  136                    "thread [%d] frame_buffer[%d].location 
is different: stack_info_1 = %ld, stack_info_2 = %ld",
  137                    i, j, si1.frame_buffer[j].location, 
si2.frame_buffer[j].location);
  138           break;
  139         }
  140       }
  141     }
  142   }

Another step would be to create functions that implement a body 
of each loop.
You can use the same techniques to simplify similar place 
(L127-L138) in the libOneGetThreadListStackTraces.c.

Thanks,
Serguei


On 7/3/20 15:55, Yasumasa Suenaga wrote:
Hi Serguei,

I'm not an Oracle employee, so I cannot know real request(s) 
from your customers.
However JDK-8201641 says Dynatrace has requested this enhancement.

BTW I haven't heared any request from my customers about this.


Thanks,

Yasumasa


On 2020/07/04 4:32, serguei.spit...@oracle.com wrote:
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