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-boun...@openjdk.java.net> On Behalf Of Yasumasa Suenaga Sent: Montag, 20. April 2020 02:33 To: serviceability-dev@openjdk.java.net Cc: yasue...@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.spit...@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.spit...@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.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 >>>>> >>> >
