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

[Yasumasa Suenaga]

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
>