Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes

[Yasumasa Suenaga]

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  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 *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::current());

There is not need in reinterpret_cast. Also, you can use the 
current_jt defined above.

Also, please, removed these two definitions as they became unus

Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes

[Chris Plummer]

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 
 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 
*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::current());


There is not need in reinterpret_cast. 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) \
 sr

Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes

[Yasumasa Suenaga]

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  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 *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::current());

There is not need in reinterpret_cast. 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/brow

RE: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes

[Reingruber, Richard]

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  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 *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 direc

Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes

[David Holmes]

Hi Yasumasa,

This looks good. A couple of minor nits below.

On 20/04/2020 10:32 am, Yasumasa Suenaga wrote:

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/


src/hotspot/share/prims/jvmtiEnvBase.hpp

This comment needs expanding now:

295   // JVMTI API helper functions which are called at safepoint or 
thread is suspended.


Can you fix up the indentation here please:

 304   jvmtiError get_current_contended_monitor(JavaThread *java_thread,
 305  jobject 
*monitor_ptr);

 306   jvmtiError get_owned_monitors(JavaThread* java_thread,
 307 
GrowableArray *owned_monitors_list);


the second lines should line up with the J in JavaThread.

Could you put the initializers on a separate line each please:

 388 : HandshakeClosure("GetOwnedMonitorInfo"),
 389   _env(env), _result(JVMTI_ERROR_NONE), 
_owned_monitors_list(owned_monitor_list) {}


Similarly for:

 428 : HandshakeClosure("GetOneCurrentContendedMonitor"),
 429   _env(env), _owned_monitor_ptr(mon_ptr),
 430   _result(JVMTI_ERROR_THREAD_NOT_ALIVE) {}

---

No need for updated webrev.

Thanks,
David
-



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 
*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::current());


There is not need in reinterpret_cast. 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 

Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes

[Yasumasa Suenaga]

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 *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::current());

There is not need in reinterpret_cast. 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

Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes

[Yasumasa Suenaga]

Thanks Serguei!

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 *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::current());

There is not need in reinterpret_cast. 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

Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes

[serguei.spit...@oracle.com]

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 
*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::current());


There is not need in reinterpret_cast. 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

Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes

[Yasumasa Suenaga]

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 *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::current());

There is not need in reinterpret_cast. 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

Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes

[serguei.spit...@oracle.com]

  
  
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
*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::current());


There is not need in reinterpret_cast. 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
  
   - 

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 *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::current());

There is not need in reinterpret_cast. 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

Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes

[serguei.spit...@oracle.com]

  
  
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 *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::current());

  There is not need in reinterpret_cast. 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