Thread Local Handshake in JVMTI functions

Thu Apr 9 06:19:42 UTC 2020

Hi Yasumasa,

I'm okay with using sub-tasks to do it incrementally.

This needs to be removed with your fix:
   src/hotspot/share/runtime/vmOperations.hpp: 
template(GetCurrentContendedMonitor)            \

Also, I agree with comments from David below:
  - GetOneCurrentContendedMonitor => GetCurrentContendedMonitor
  - Thread::current() =>  calling_thread

I hope, you will update the copyright comments in jvmtiEnvBase.?pp.

Are you going to re-post this as an RFR with a correct sub-task number?

Thanks,
Serguei

On 4/8/20 22:52, David Holmes wrote:
> 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
>>>>>>>
>>>>>>