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

[David Holmes]

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

> 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.

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.

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

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

why not?

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();

> 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.

> 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