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

Yasumasa Suenaga suenaga at oss.nttdata.com
Thu Apr 23 00:00:46 UTC 2020 

Hi Chris,

I filed it to JBS:
   https://bugs.openjdk.java.net/browse/JDK-8243450

I will send review request later.


Thanks,

Yasumasa


On 2020/04/23 5:02, Chris Plummer wrote:
> Hi Yasumasa,
> 
> Yes, it looks like VMOps in SA can be removed. It's probably bit rotted code. My guess is at some point there was support, or were plans for supporting, the debugging of VMOps from within SA. Given that there are no references to the VMOps class, it looks like none of that support currently exists.
> 
> thanks,
> 
> Chris
> 
> On 4/20/20 5:24 PM, Yasumasa Suenaga wrote:
>> 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
>>>>>>>>
>>>>>>
>>>>
> 
>

More information about the serviceability-dev mailing list