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

[Yasumasa Suenaga]

Thanks Serguei!

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
>>>>>
>>>
>