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

[David Holmes]

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.

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

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.

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

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