Thread Local Handshake in JVMTI functions

[David Holmes]

Hi Yasumasa,

On 9/04/2020 3:08 pm, Yasumasa Suenaga wrote:
> Hi,
> 
> JDK-8240918 has been pushed, so I made a patch for 
> GetCurrentContendedMonitor(). How about this?
> 
>    http://cr.openjdk.java.net/~ysuenaga/jvmti-thread-local-handshake/2/

Generally looks okay. A couple of comments:

src/hotspot/share/prims/jvmtiEnv.cpp

Please update the comment just before the change:

    // thread. All other usage needs to use a vm-safepoint-op for safety.

---

  GetOneCurrentContendedMonitor

I don't think you need to add One into the name here - the current 
contended monitor is inherently a singleton.

---

src/hotspot/share/prims/jvmtiEnvBase.cpp

655           Thread::current() == java_thread->active_handshaker() ||

The calling_thread parameter is now the current thread. So this can also 
be changed:

679     Handle     hobj(Thread::current(), obj);



>>>>>>>> An observation, it seems to me that calling_thread is not used 
>>>>>>>> when this is not a VMOperation.
> 
> calling_thread is used for creating JNI reference:
> 
> ```
> *monitor_ptr = jni_reference(calling_thread, hobj);
> ```
> 
> 
> If it is ok, I will make patches for other JVMTI functions.
> However the patch might be bigger, so I want to separate as below. What 
> do you think?
> (I think we can file them as subtask under JDK-8201641)
> 
> 
> * Monitor operation
>      - VM_GetOwnedMonitorInfo
>      - VM_GetCurrentContendedMonitor
> 
> * Frame operation
>      - VM_UpdateForPopTopFrame
>      - VM_SetFramePop
> 
> * Thread operation
>      - VM_GetFrameCount
>      - VM_GetFrameLocation
>      - VM_GetThreadListStackTraces
>      - VM_GetCurrentLocation
>      - VM_EnterInterpOnlyMode

Creating subtasks with separate RFRs is fine by me. It may be good to 
slowly introduce these changes so that we get some testing coverage to 
iron out any initial bugs with the approach.

Thanks,
David
-----

> 
> Thanks,
> 
> Yasumasa
> 
> 
> On 2020/04/01 21:29, Yasumasa Suenaga wrote:
>> Thanks David!
>>
>> If JDK-8201641 is not assigned when JDK-8240918 is resolved, I will 
>> start to work for JDK-8201641.
>> (It would be large patch...)
>>
>>
>> Cheers,
>>
>> Yasumasa
>>
>>
>> On 2020/04/01 19:05, David Holmes wrote:
>>> On 1/04/2020 11:02 am, Yasumasa Suenaga wrote:
>>>> Thanks Dan and Serguei!
>>>>
>>>> I added a comment for this to JDK-8201641.
>>>>
>>>> David, can you share Bug ID for thread-to-thread handshake?
>>>> I want to record it to JDK-8201641 as a blocker.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8240918
>>>
>>> I heard the RFR could be as soon as tomorrow :)
>>>
>>> Cheers,
>>> David
>>>
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2020/04/01 1:59, serguei.spitsyn at oracle.com wrote:
>>>>> Hi Yasumasa,
>>>>>
>>>>> Yes, this works needs to be done.
>>>>> I'll take look at you webrev.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>> On 3/31/20 07:41, Daniel D. Daugherty wrote:
>>>>>> Add Robbin to this thread...
>>>>>>
>>>>>>
>>>>>> This reminded of the following RFE that Robbin filed:
>>>>>>
>>>>>>     JDK-8201641 JVMTI: GetThreadListStackTraces should use 
>>>>>> Thread-Local Handshakes
>>>>>>     https://bugs.openjdk.java.net/browse/JDK-8201641
>>>>>>
>>>>>> We could update 8201641 to include everything that Yasumasa-san is 
>>>>>> requesting.
>>>>>> Would be a good place to track it...
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>> On 3/31/20 7:40 AM, Yasumasa Suenaga wrote:
>>>>>>> Hi David,
>>>>>>>
>>>>>>> On 2020/03/31 19:16, David Holmes wrote:
>>>>>>>> Hi Yasumasa,
>>>>>>>>
>>>>>>>> On 31/03/2020 8:06 pm, Yasumasa Suenaga wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> Many JVMTI functions uses VM Operation to get information. 
>>>>>>>>> However some of them need to stop only one thread - they don't 
>>>>>>>>> need to stop all threads.
>>>>>>>>> So I think we can use Thread Local Handshake as this webrev. It 
>>>>>>>>> is example for GetOneCurrentContendedMonitor().
>>>>>>>>
>>>>>>>> True, but at the moment handshakes involve the VMThread. There 
>>>>>>>> is work being done to support direct thread-to-thread handshakes 
>>>>>>>> and once that is done this kind of conversion should be more 
>>>>>>>> easily done. It might be worth waiting for that.
>>>>>>>
>>>>>>> Thanks, I will be back to this topic when thread-to-thread 
>>>>>>> handshake is done.
>>>>>>> I wondered at first why VMThread involves handshake. Its 
>>>>>>> improvement is welcome for me ;)
>>>>>>>
>>>>>>>
>>>>>>> Cheers,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/jvmti-thread-local-handshake/
>>>>>>>>
>>>>>>>> An observation, it seems to me that calling_thread is not used 
>>>>>>>> when this is not a VMOperation.
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> David
>>>>>>>>
>>>>>>>>> Also I think we can replace following VM Operations to Thread 
>>>>>>>>> Local Handshake:
>>>>>>>>>
>>>>>>>>> class VM_GetCurrentLocation
>>>>>>>>> class VM_EnterInterpOnlyMode
>>>>>>>>> class VM_UpdateForPopTopFrame
>>>>>>>>> class VM_SetFramePop
>>>>>>>>> class VM_GetOwnedMonitorInfo
>>>>>>>>> class VM_GetCurrentContendedMonitor
>>>>>>>>> class VM_GetFrameCount
>>>>>>>>> class VM_GetFrameLocation
>>>>>>>>>
>>>>>>>>> What do you think?
>>>>>>>>> It it is acceptable, I will file it to JBS and send review 
>>>>>>>>> request.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>
>>>>>