Hi Chris,
I filed it to JBS:
https://bugs.openjdk.java.net/browse/JDK-8243450
I will send review request later.
Thanks,
Yasumasa
On 2020/04/23 5:02, Chris Plummer wrote:
Hi Yasumasa,
Yes, it looks like VMOps in SA can be removed. It's probably bit rotted code.
My guess is at some point there was support, or were plans for supporting, the
debugging of VMOps from within SA. Given that there are no references to the
VMOps class, it looks like none of that support currently exists.
thanks,
Chris
On 4/20/20 5:24 PM, Yasumasa Suenaga wrote:
Hi Rechard,
Thank you for telling it to me.
I grep'ed jdk.hotspot.agent with VMOps (it is just enum), it does not seem to
be used.
In addition, VMOps has not already synced to HotSpot. For example,
VM_HandshakeOneThread does not appear in VMOps.
(I wonder how do we use VMOps in SA)
Thus I think we can (should) remove VMOps from jdk.hotspot.agent .
If other serviceability folks agree with it, I will file it to JBS.
Thanks,
Yasumasa
On 2020/04/21 0:02, Reingruber, Richard wrote:
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