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
