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

[serguei.spitsyn at oracle.com]

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