RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes

Tue Apr 21 00:24:04 UTC 2020

Hi Rechard,

Thank you for telling it to me.

I grep'ed jdk.hotspot.agent with VMOps (it is just enum), it does not seem to be used.
In addition, VMOps has not already synced to HotSpot. For example, VM_HandshakeOneThread does not appear in VMOps.
(I wonder how do we use VMOps in SA)

Thus I think we can (should) remove VMOps from jdk.hotspot.agent .
If other serviceability folks agree with it, I will file it to JBS.

Thanks,

Yasumasa

On 2020/04/21 0:02, Reingruber, Richard wrote:
> Hi Yasumasa,
> 
> I'm obviously late to this review. Still I wanted to let you know, that there's another file in the
> source tree that lists the VM operations in the VM (just found out about it myself):
> 
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VMOps.java
> 
> Probably you should remove the VM operations from that file too.
> 
> It seems to be part of the serviceability agent, which I hardly know. Would be good if somebody
> who is more familiar with it could let us know...
> 
> Best regards,
> Richard.
> 
> -----Original Message-----
> From: serviceability-dev <serviceability-dev-bounces at openjdk.java.net> On Behalf Of Yasumasa Suenaga
> Sent: Montag, 20. April 2020 02:33
> To: serviceability-dev at openjdk.java.net
> Cc: yasuenag at gmail.com
> Subject: Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes
> 
> Hi all,
> 
> Could you review it?
> 
>     JBS: https://bugs.openjdk.java.net/browse/JDK-8242425
>     webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.02/
> 
> I need one more reviewer to push.
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> On 2020/04/17 5:13, serguei.spitsyn at oracle.com wrote:
>> Hi Yasumasa,
>>
>> Thank you for the update.
>> It looks good.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 4/10/20 04:30, Yasumasa Suenaga wrote:
>>> Hi Serguei,
>>>
>>> I use current_jt in this webrev. Could you review again?
>>>
>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.02/
>>>
>>> I tested this change with vmTestbase/nsk/jvmti, they are fine on my Linux x64.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/04/10 17:21, serguei.spitsyn at oracle.com wrote:
>>>> Hi Yasumasa,
>>>>
>>>> Thank you for the update.
>>>>
>>>> Minor:
>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.01/src/hotspot/share/prims/jvmtiEnvBase.cpp.udiff.html
>>>>
>>>> + err = get_locked_objects_in_frame(JavaThread::current(), java_thread, jvf, owned_monitors_list, depth-1); . . .
>>>>
>>>> + JvmtiMonitorClosure jmc(java_thread, JavaThread::current(), owned_monitors_list, this);
>>>>
>>>> You can use current_jt instead of JavaThread::current() above.
>>>>
>>>> There is also some test coverage in the vmTestbase/nsk/jvmti/scenarios tests.
>>>> This one as well:
>>>> test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/functions/nosuspendMonitorInfo/JvmtiTest
>>>> Probably it is easier to run all vmTestbase/nsk/jvmti tests.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 4/10/20 01:05, Yasumasa Suenaga wrote:
>>>>> Hi Serguei,
>>>>>
>>>>> Thanks for your comment!
>>>>> I uploaded new webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.01/
>>>>>
>>>>> I ran following tests, and all of them were passed on my Linux x64.
>>>>>
>>>>>   - vmTestbase/nsk/jvmti/GetCurrentContendedMonitor
>>>>>   - vmTestbase/nsk/jvmti/GetOwnedMonitorInfo
>>>>>   - vmTestbase/nsk/jdi
>>>>>   - vmTestbase/nsk/jdwp
>>>>>   - serviceability/jvmti/GetOwnedMonitorInfo
>>>>>   - serviceability/jvmti/GetOwnedMonitorStackDepthInfo
>>>>>   - serviceability/jdwp
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2020/04/10 14:50, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Yasumasa,
>>>>>>
>>>>>> It looks pretty good in general.
>>>>>> A couple of comments though.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.00/src/hotspot/share/prims/jvmtiEnvBase.cpp.frames.html
>>>>>>
>>>>>> 650 JvmtiEnvBase::get_current_contended_monitor(JavaThread *java_thread, jobject *monitor_ptr) {
>>>>>> 651 assert(!Thread::current()->is_VM_thread() &&
>>>>>> 652 (Thread::current() == java_thread ||
>>>>>> 653 Thread::current() == java_thread->active_handshaker()),
>>>>>> 654 "call by myself or at direct handshake");
>>>>>>
>>>>>>    ...
>>>>>>
>>>>>> 685 JvmtiEnvBase::get_owned_monitors(JavaThread* java_thread,
>>>>>>    686 GrowableArray<jvmtiMonitorStackDepthInfo*> *owned_monitors_list) {
>>>>>>    687   jvmtiError err = JVMTI_ERROR_NONE;
>>>>>> 688 assert(!Thread::current()->is_VM_thread() &&
>>>>>> 689 (Thread::current() == java_thread ||
>>>>>> 690 Thread::current() == java_thread->active_handshaker()),
>>>>>> 691 "call by myself or at direct handshake");
>>>>>>
>>>>>> I'd suggest to add this at the beginning:
>>>>>>     JavaThread *current_jt = JavaThread::current();
>>>>>>
>>>>>>
>>>>>> 676 JavaThread *current_jt = reinterpret_cast<JavaThread *>(JavaThread::current());
>>>>>>
>>>>>> There is not need in reinterpret_cast<JavaThread *>. Also, you can use the current_jt defined above.
>>>>>>
>>>>>> Also, please, removed these two definitions as they became unused with your fix:
>>>>>>      src/hotspot/share/runtime/vmOperations.hpp: template(GGetCurrentContendedMonitoretOwnedMonitorInfo) \
>>>>>>      src/hotspot/share/runtime/vmOperations.hpp: template()            \
>>>>>>
>>>>>> The impacted JVMTI monitor functions are used in the JDWP agent to support the JDI ThreadReference implementation.
>>>>>> To be safe the JDI & JDWP tests have to be run as well. It is well covered by the tier-5.
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>> On 4/9/20 21:46, Yasumasa Suenaga wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Please review this change:
>>>>>>>
>>>>>>>    JBS: https://bugs.openjdk.java.net/browse/JDK-8242425
>>>>>>>    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.00/
>>>>>>>
>>>>>>> We've discussed to use Thread-Local Handshake in some JVMTI functions [1].
>>>>>>> This change is for monitor functions. It affects GetOwnedMonitorInfo(), GetOwnedMonitorStackDepthInfo(), GetCurrentContendedMonitor().
>>>>>>>
>>>>>>> It passed tests on submit repo (mach5-one-ysuenaga-JDK-8242425-20200410-0228-10075639), and also I confirmed to pass following tests:
>>>>>>>
>>>>>>>   - serviceability/jvmti/GetOwnedMonitorInfo
>>>>>>>   - serviceability/jvmti/GetOwnedMonitorStackDepthInfo
>>>>>>>   - vmTestbase/nsk/jvmti/GetCurrentContendedMonitor
>>>>>>>   - vmTestbase/nsk/jvmti/GetOwnedMonitorInfo
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>> [1] https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-March/030890.html
>>>>>>
>>>>
>>