Re: 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.spit...@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