On 2020/04/09 15:44, serguei.spit...@oracle.com wrote:
On 4/8/20 23:36, Yasumasa Suenaga wrote:
Thanks David and Serguei!

I created 3 subtasks under JDK-8201641, of course I will send review request in 
each them.

  - GetOneCurrentContendedMonitor => GetCurrentContendedMonitor

`GetCurrentContendedMonitor` is JVMTI function name, and also it exists as 
public member of JvmtiEnv class.
So I want to different name for HandshakeClosure - for example, 
GetCurrentContendedMonitorClosure.

Do you have any idea for it?

It is better to follow the same pattern as we already have in other places 
(until a decision is made about a different rule).
So, the GetCurrentContendedMonitorClosure should work for now.
It is interesting if there will be any other suggestions though.

Currently, most of classes which extend HandshakeClosure have "Closure" as 
suffix in below:

```
$ grep -nrw HandshakeClosure * | grep -w class
share/gc/shenandoah/shenandoahUnload.cpp:161:class 
ShenandoahUnloadRendezvousClosure : public HandshakeClosure {
share/gc/z/zHeap.cpp:333:class ZRendezvousClosure : public HandshakeClosure {
share/gc/z/zMark.cpp:422:class ZMarkFlushAndFreeStacksClosure : public 
HandshakeClosure {
share/prims/whitebox.cpp:2046:  class TraceSelfClosure : public 
HandshakeClosure {
share/runtime/biasedLocking.cpp:505:class RevokeOneBias : public 
HandshakeClosure {
share/runtime/deoptimization.cpp:808:class DeoptimizeMarkedClosure : public 
HandshakeClosure {
share/runtime/handshake.hpp:44:class HandshakeClosure : public ThreadClosure {
share/runtime/sweeper.cpp:200:class NMethodMarkingClosure : public 
HandshakeClosure {
share/runtime/thread.cpp:528:class InstallAsyncExceptionClosure : public 
HandshakeClosure {
share/runtime/vmThread.cpp:391:class HandshakeALotClosure : public 
HandshakeClosure {
```

I think `GetCurrentContendedMonitorClosure` is not so bad :)


Thanks,

Yasumasa


Thanks,
Serguei


Yasumasa


On 2020/04/09 15:19, serguei.spit...@oracle.com wrote:
Hi Yasumasa,

I'm okay with using sub-tasks to do it incrementally.

This needs to be removed with your fix:
   src/hotspot/share/runtime/vmOperations.hpp: 
template(GetCurrentContendedMonitor)            \

Also, I agree with comments from David below:
  - GetOneCurrentContendedMonitor => GetCurrentContendedMonitor
  - Thread::current() =>  calling_thread

I hope, you will update the copyright comments in jvmtiEnvBase.?pp.

Are you going to re-post this as an RFR with a correct sub-task number?

Thanks,
Serguei


On 4/8/20 22:52, David Holmes wrote:
Hi Yasumasa,

On 9/04/2020 3:08 pm, Yasumasa Suenaga wrote:
Hi,

JDK-8240918 has been pushed, so I made a patch for 
GetCurrentContendedMonitor(). How about this?

http://cr.openjdk.java.net/~ysuenaga/jvmti-thread-local-handshake/2/

Generally looks okay. A couple of comments:

src/hotspot/share/prims/jvmtiEnv.cpp

Please update the comment just before the change:

   // thread. All other usage needs to use a vm-safepoint-op for safety.

---

 GetOneCurrentContendedMonitor

I don't think you need to add One into the name here - the current contended 
monitor is inherently a singleton.

---

src/hotspot/share/prims/jvmtiEnvBase.cpp

655           Thread::current() == java_thread->active_handshaker() ||

The calling_thread parameter is now the current thread. So this can also be 
changed:

679     Handle     hobj(Thread::current(), obj);



An observation, it seems to me that calling_thread is not used when this is not 
a VMOperation.

calling_thread is used for creating JNI reference:

```
*monitor_ptr = jni_reference(calling_thread, hobj);
```


If it is ok, I will make patches for other JVMTI functions.
However the patch might be bigger, so I want to separate as below. What do you 
think?
(I think we can file them as subtask under JDK-8201641)


* Monitor operation
     - VM_GetOwnedMonitorInfo
     - VM_GetCurrentContendedMonitor

* Frame operation
     - VM_UpdateForPopTopFrame
     - VM_SetFramePop

* Thread operation
     - VM_GetFrameCount
     - VM_GetFrameLocation
     - VM_GetThreadListStackTraces
     - VM_GetCurrentLocation
     - VM_EnterInterpOnlyMode

Creating subtasks with separate RFRs is fine by me. It may be good to slowly 
introduce these changes so that we get some testing coverage to iron out any 
initial bugs with the approach.

Thanks,
David
-----


Thanks,

Yasumasa


On 2020/04/01 21:29, Yasumasa Suenaga wrote:
Thanks David!

If JDK-8201641 is not assigned when JDK-8240918 is resolved, I will start to 
work for JDK-8201641.
(It would be large patch...)


Cheers,

Yasumasa


On 2020/04/01 19:05, David Holmes wrote:
On 1/04/2020 11:02 am, Yasumasa Suenaga wrote:
Thanks Dan and Serguei!

I added a comment for this to JDK-8201641.

David, can you share Bug ID for thread-to-thread handshake?
I want to record it to JDK-8201641 as a blocker.

https://bugs.openjdk.java.net/browse/JDK-8240918

I heard the RFR could be as soon as tomorrow :)

Cheers,
David


Yasumasa


On 2020/04/01 1:59, serguei.spit...@oracle.com wrote:
Hi Yasumasa,

Yes, this works needs to be done.
I'll take look at you webrev.

Thanks,
Serguei

On 3/31/20 07:41, Daniel D. Daugherty wrote:
Add Robbin to this thread...


This reminded of the following RFE that Robbin filed:

    JDK-8201641 JVMTI: GetThreadListStackTraces should use Thread-Local 
Handshakes
https://bugs.openjdk.java.net/browse/JDK-8201641

We could update 8201641 to include everything that Yasumasa-san is requesting.
Would be a good place to track it...

Dan


On 3/31/20 7:40 AM, Yasumasa Suenaga wrote:
Hi David,

On 2020/03/31 19:16, David Holmes wrote:
Hi Yasumasa,

On 31/03/2020 8:06 pm, Yasumasa Suenaga wrote:
Hi all,

Many JVMTI functions uses VM Operation to get information. However some of them 
need to stop only one thread - they don't need to stop all threads.
So I think we can use Thread Local Handshake as this webrev. It is example for 
GetOneCurrentContendedMonitor().

True, but at the moment handshakes involve the VMThread. There is work being 
done to support direct thread-to-thread handshakes and once that is done this 
kind of conversion should be more easily done. It might be worth waiting for 
that.

Thanks, I will be back to this topic when thread-to-thread handshake is done.
I wondered at first why VMThread involves handshake. Its improvement is welcome 
for me ;)


Cheers,

Yasumasa


http://cr.openjdk.java.net/~ysuenaga/jvmti-thread-local-handshake/

An observation, it seems to me that calling_thread is not used when this is not 
a VMOperation.

Cheers,
David

Also I think we can replace following VM Operations to Thread Local Handshake:

class VM_GetCurrentLocation
class VM_EnterInterpOnlyMode
class VM_UpdateForPopTopFrame
class VM_SetFramePop
class VM_GetOwnedMonitorInfo
class VM_GetCurrentContendedMonitor
class VM_GetFrameCount
class VM_GetFrameLocation

What do you think?
It it is acceptable, I will file it to JBS and send review request.


Thanks,

Yasumasa




Reply via email to