Thread Local Handshake in JVMTI functions

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Thu Apr 9 06:44:45 UTC 2020 

On 4/8/20 23:36, Yasumasa Suenaga wrote:
> Thanks David and Serguei!
>
> I created 3 subtasks under JDK-8201641, of course I will send review 
> request in each them.
>
>>   - GetOneCurrentContendedMonitor => GetCurrentContendedMonitor
>
> `GetCurrentContendedMonitor` is JVMTI function name, and also it 
> exists as public member of JvmtiEnv class.
> So I want to different name for HandshakeClosure - for example, 
> GetCurrentContendedMonitorClosure.
>
> Do you have any idea for it?

It is better to follow the same pattern as we already have in other 
places (until a decision is made about a different rule).
So, the GetCurrentContendedMonitorClosure should work for now.
It is interesting if there will be any other suggestions though.

Thanks,
Serguei

>
> Yasumasa
>
>
> On 2020/04/09 15:19, serguei.spitsyn at oracle.com wrote:
>> 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
>>>>>>>>>
>>>>>>>>
>>

More information about the serviceability-dev mailing list