Hi David,

On 2020/06/25 21:48, David Holmes wrote:
Hi Yasumasa,

On 25/06/2020 6:24 pm, Yasumasa Suenaga wrote:
Hi David,

Thanks for your comment!

On 2020/06/25 14:17, David Holmes wrote:
Hi Yasumasa,

Thanks for tackling this. I've had an initial look at it and have a few 
concerns.

On 24/06/2020 4:50 pm, 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/

Some typos:

invaliant -> invariant
directry -> directly

I will fix them.


This change replace following VM operations to direct handshake.

  - VM_GetFrameCount (GetFrameCount())
  - VM_GetFrameLocation (GetFrameLocation())
  - VM_GetThreadListStackTraces (GetThreadListStackTrace())
  - VM_GetCurrentLocation

It would have been better to split these out into separate changes. I am 
finding it very hard to track through the webrev and try to compare the old 
safepoint based operation with the new direct handshake approach, to check they 
are functionally equivalent.

I will separate them as following. What do you think?
If you are ok, I will update JBS.

  - Thread operations
      - VM_GetThreadListStackTraces (GetThreadListStackTrace())
      - VM_GetStackTrace(GetStackTrace())  <- I missed it to describe in 
previous mail, sorry.

  - Frame operations
      - VM_GetFrameCount (GetFrameCount())
      - VM_GetFrameLocation (GetFrameLocation())
      - VM_GetCurrentLocation

I will start to work when they are separated.

If the frame operations are each small enough that will help.

I updated JBS as above.


You are not checking the return value of Handshake::execute_direct and so are 
missing the possibility that the target thread has terminated before you got to 
do the operation on it. It isn't clear to me under what other circumstances 
execute_direct can also return false.

I will add it. According to Handshake::execute_direct() and 
HandshakeOperation::do_handshake(), it seems to return false if the target 
thread has terminated as you said.

Yes, but also if the handshake is not executed - but I don't know under what 
conditions that can occur.


You don't seem to have these checks anymore in some places:

   && !_java_thread->is_exiting() && _java_thread->threadObj() != NULL)

why not?

I thought the thread which enters handshake is always alive and it has 
threadObj.

As far as I can see we can still engage in a handshake with a thread after it 
has marked itself as exiting.

I think the handshake should not be run if its state is exiting because we can deem it as 
"dead".
What do you think?


The threadObj() can only be null while a thread is attaching, which means it 
would have to checked in the general case, but for these JVM TI operations if 
we already have a jthread reference to the target thread then it must be beyond 
that point. Mind you that same logic applies to the existing code so ...

I will recover their conditions.
(I also should recover them for GetOwnedMonitorInfoClosure and 
GetCurrentContendedMonitorClosure - I removed them in JDK-8242425)

I think so - and we need to check the return value of execute_direct to 
determine when to report JVMTI_ERROR_THREAD_NOT_ALIVE.

I will file it to JBS.
We can get the result from result() in their Closures. 
JVMTI_ERROR_THREAD_NOT_ALIVE is set by default in 
GetCurrentContendedMonitorClosure, so we can get this error if the handshake is 
not completed.
Should I check result of execute_direct() even if that?
(Of course, we should fix GetOwnedMonitorInfoClosure and threadObj() check)


It is not clear that all the code that previously could execute at a safepoint, 
due to being called from a VM_Operation, is still executable at a safepoint 
e.g. JvmtiThreadState::count_frames()

GetThreadListStackTrace() uses direct handshake if thread count == 1. In other 
case (thread count > 1), it would be performed as VM operation 
(VM_GetThreadListStackTraces).

This introduces a large chunk of duplicated code for the frame fill in and 
final allocation. Can you not reuse the existing logic that does this - and in 
the process do away with the the use of _needs_thread_state? I really wanted to 
see simpler code after this conversion.

I'm also wondering whether we can hide all this logic in the closure, as was 
done with the VM_Operation i.e.

*stack_info_ptr = op.stack_info();

I will try to refactor this change.


Caller of VM_GetCurrentLocation (JvmtiEnvThreadState::reset_current_location()) 
might be called at safepoint. So I added safepoint check in its caller.

I could not figure out what you were referring to here.

I guess following callpath is available:

VM_GetCurrentLocation
   JvmtiEnvThreadState::reset_current_location()
     JvmtiEventControllerPrivate::recompute_env_thread_enabled()
       JvmtiEventControllerPrivate::recompute_thread_enabled()
         JvmtiEventControllerPrivate::set_frame_pop()
           JvmtiEventController::set_frame_pop()
             JvmtiEnvThreadState::set_frame_pop()
               VM_SetFramePop::doit()

However, VM_SetFramePop seems not to allow nested VM operations.

It is the outer operation that has to allow nesting but VM_GetCurrentLocation 
doesn't allow it either. So if this path is possible then something is broken.

I'm not sure this path would be happen. However following comments are left in 
the code:

```
      // The java thread stack may not be walkable for a running thread
      // so get current location at safepoint.
      VM_GetCurrentLocation op(_thread);
```


Thanks,

Yasumasa


Cheers,
David


This change has been tested in serviceability/jvmti serviceability/jdwp 
vmTestbase/nsk/jvmti vmTestbase/nsk/jdi vmTestbase/ns
k/jdwp.

Just a general comment on testing for these conversions to direct handshakes. 
We have no control over whether the handshake gets executed in the original 
thread or the target thread, so for all we know all our testing could be 
executing only one of the cases. This concerns me but I am not yet sure what to 
do about it.

Thanks,
David
-----

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