[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
From:       Yasumasa Suenaga <suenaga () oss ! nttdata ! com>
Date:       2020-06-30 23:58:34
Message-ID: f155de52-5f16-15c4-eacb-1f1b0ec3f58e () oss ! nttdata ! com
[Download RAW message or body]

On 2020/07/01 8:54, David Holmes wrote:
> On 1/07/2020 9:51 am, Yasumasa Suenaga wrote:
> > Hi Serguei,
> > 
> > On 2020/07/01 8:24, serguei.spitsyn@oracle.com wrote:
> > > Hi Yasumasa,
> > > 
> > > Thank you for separating your initial webrev.
> > > I'll do a full review after you address comments from David and Robbin as I'm \
> > > stepping on the same ground. 
> > > Just a quick comment now.
> > > 
> > > http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/src/hotspot/share/prims/jvmtiEnv.cpp.udiff.html
> > >  
> > > I've already asked in prev. round to make this renaming: target_javathread => \
> > > java_thread The identifier java_thread is normally used in the jvmtiEnv.cpp \
> > > functions. The target_javathread sounds very unusual.
> > 
> > Sorry, I've fixed it. I will update webrev.
> > 
> > 
> > > I do not like much the introduction ofthe GetSingleStackTraceClosure.
> > > It feels like it can be done in a more elegant way.
> > > From the other hand, it is not that bad. :-)
> > 
> > I thought to use handshake on VMThread (!= direct handshake) for \
> > GetThreadListStackTraces() to be simplify implementation. However it would be \
> > queued as VM op (of course STW would not happen), so I introduced \
> > GetSingleStackTraceClosure.
> 
> Getting multiple stacktraces must be a stop-the-world operation, so that the traces \
> are consistent.

Yes, this is the case of thread_count == 1.

Yasumasa


> David
> 
> > 
> > Thanks,
> > 
> > Yasumasa
> > 
> > 
> > > Thanks,
> > > Serguei
> > > 
> > > 
> > > On 6/29/20 17:05, Yasumasa Suenaga wrote:
> > > > Hi David, Serguei,
> > > > 
> > > > I updated webrev for 8242428. Could you review again?
> > > > This change migrate to use direct handshake for GetStackTrace() and \
> > > > GetThreadListStackTraces() (when thread_count == 1). 
> > > > http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/
> > > > 
> > > > VM_GetThreadListStackTrace (for GetThreadListStackTraces) and \
> > > > VM_GetAllStackTraces (for GetAllStackTraces) have inherited \
> > > > VM_GetMultipleStackTraces VM operation which provides the feature to generate \
> > > > jvmtiStackInfo. I modified  VM_GetMultipleStackTraces to a normal C++ class \
> > > > to share with HandshakeClosure for GetThreadListStackTraces \
> > > > (GetSingleStackTraceClosure). 
> > > > Also I added new testcases which test GetThreadListStackTraces() with \
> > > > thread_count == 1 and with all threads. 
> > > > This change has been tested in serviceability/jvmti serviceability/jdwp \
> > > > vmTestbase/nsk/jvmti vmTestbase/nsk/jdi vmTestbase/nsk/jdwp. 
> > > > 
> > > > Thanks,
> > > > 
> > > > Yasumasa
> > > > 
> > > > 
> > > > On 2020/06/24 15:50, Yasumasa Suenaga wrote:
> > > > > Hi all,
> > > > > 
> > > > > Please review this change:
> > > > > 
> > > > > JBS: https://bugs.openjdk.java.net/browse/JDK-8242428
> > > > > webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/
> > > > > 
> > > > > This change replace following VM operations to direct handshake.
> > > > > 
> > > > > - VM_GetFrameCount (GetFrameCount())
> > > > > - VM_GetFrameLocation (GetFrameLocation())
> > > > > - VM_GetThreadListStackTraces (GetThreadListStackTrace())
> > > > > - VM_GetCurrentLocation
> > > > > 
> > > > > GetThreadListStackTrace() uses direct handshake if thread count == 1. In \
> > > > > other case (thread count > 1), it would be performed as VM operation \
> > > > > (VM_GetThreadListStackTraces). Caller of VM_GetCurrentLocation \
> > > > > (JvmtiEnvThreadState::reset_current_location()) might be called at \
> > > > > safepoint. So I added safepoint check in its caller. 
> > > > > This change has been tested in serviceability/jvmti serviceability/jdwp \
> > > > > vmTestbase/nsk/jvmti vmTestbase/nsk/jdi vmTestbase/ns k/jdwp.
> > > > > 
> > > > > Also I tested it on submit repo, then it has execution error \
> > > > > (mach5-one-ysuenaga-JDK-8242428-20200624-0054-12034717) due to dependency \
> > > > > error. So I think it does not occur by this change. 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > Yasumasa
> > > 


[prev in list] [next in list] [prev in thread] [next in thread]