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
