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

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8248362: JVMTI frame operations should use Thread-Local Handshake
From:       Yasumasa Suenaga <suenaga () oss ! nttdata ! com>
Date:       2020-07-25 2:19:25
Message-ID: 3fb80424-9234-0a4f-1ec5-2c7f69854ae8 () oss ! nttdata ! com
[Download RAW message or body]

Thanks Dan!

Yasumasa


On 2020/07/25 3:24, Daniel D. Daugherty wrote:
> On 7/22/20 11:12 PM, Yasumasa Suenaga wrote:
> > Hi Dan,
> > 
> > Thanks for your comment!
> > I uploaded new webrev:
> > 
> > http://cr.openjdk.java.net/~ysuenaga/JDK-8248362/webrev.01/
> > Diff from previous webrev: http://hg.openjdk.java.net/jdk/submit/rev/df75038b5449
> 
> Thumbs up. Reviewed by comparing the previous webrev patch (webrev.00) with
> the latest (webrev.01).
> 
> Dan
> 
> 
> > 
> > On 2020/07/23 10:04, Daniel D. Daugherty wrote:
> > > On 7/22/20 10:38 AM, Yasumasa Suenaga wrote:
> > > > Hi all,
> > > > 
> > > > Please review this change:
> > > > 
> > > > JBS: https://bugs.openjdk.java.net/browse/JDK-8248362
> > > > webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8248362/webrev.00/
> > > 
> > > src/hotspot/share/prims/jvmtiEnv.cpp
> > > L1755     // JVMTI get java stack frame location at safepoint.
> > > You missed updating this one. Perhaps:
> > > 
> > > // JVMTI get java stack frame location via direct handshake.
> > 
> > Fixed.
> > 
> > 
> > > src/hotspot/share/prims/jvmtiEnvBase.cpp
> > > L1563:   JavaThread* jt = _state->get_thread();
> > > L1564:   assert(target == jt, "just checking");
> > > This code is different than the other closures. It might be worth
> > > a comment to explain why. I don't remember why VM_GetFrameCount had
> > > to use the JvmtiThreadState to fetch the JavaThread. Serguei might
> > > remember.
> > > 
> > > It could be that we don't need the _state field anymore because of
> > > the way that handshakes work (and provide the JavaThread* to the
> > > do_thread() function). Your choice on whether to deal with that as
> > > part of this fix or following with another RFE.
> > > 
> > > Update: Uggg.... the get_frame_count() function takes JvmtiThreadState
> > > as a parameter. This is very much entangled... sigh... we should
> > > definitely look at untangling this in another RFE...
> > 
> > I want to fix it in another RFE as you said if it is needed.
> > If we don't hear the reason from Serguei, I want to keep this change.
> > 
> > 
> > > old L1565:   ThreadsListHandle tlh;
> > > new L1565:   if (!jt->is_exiting() && jt->threadObj() != NULL) {
> > > 
> > > Please consider add this comment above L1565:
> > > // ThreadsListHandle is not needed due to direct handshake.
> > > 
> > > Yes, I see that previous closures were added without that comment,
> > > but when I see "if (!jt->is_exiting() && jt->threadObj() != NULL)"
> > > I immediately wonder where the ThreadsListHandle is... Please consider
> > > adding the comment to the others also.
> > > 
> > > old L1574:   ThreadsListHandle tlh;
> > > new L1574:   if (!jt->is_exiting() && jt->threadObj() != NULL) {
> > > 
> > > Please consider add this comment above L1574:
> > > // ThreadsListHandle is not needed due to direct handshake.
> > 
> > I think it is new normal as David said in the reply, and also other closures \
> > don't say about it. So I did not change about it in new webrev.
> > 
> > 
> > > src/hotspot/share/prims/jvmtiEnvBase.hpp
> > > L580: // HandshakeClosure to frame location.
> > > typo - s/to frame/to get frame/
> > 
> > Fixed.
> > 
> > 
> > > src/hotspot/share/prims/jvmtiThreadState.cpp
> > > No comments on the changes.
> > > 
> > > For the above comments about VM_GetFrameCount, understanding why
> > > JvmtiThreadState::count_frames() has to exist in JvmtiThreadState
> > > will go along way towards untangling the mess.
> > > 
> > > src/hotspot/share/runtime/vmOperations.hpp
> > > No comments.
> > > 
> > > 
> > > Thumbs up. I only have nits above. If you choose to make the above
> > > changes, I do not need to see another webrev.
> > 
> > Thanks, but I uploaded new webrev just in case :)
> > 
> > 
> > Yasumasa
> > 
> > 
> > > Dan
> > > 
> > > 
> > > 
> > > > 
> > > > Migrate JVMTI frame operations to Thread-Local Handshakes from VM Operations.
> > > > 
> > > > - VM_GetFrameCount
> > > > - VM_GetFrameLocation
> > > > 
> > > > They affects both GetFrameCount() and GetFrameLocation() JVMTI functions.
> > > > 
> > > > This change has passed all tests on submit repo \
> > > > (mach5-one-ysuenaga-JDK-8248362-20200722-1249-12859056), and \
> > > > vmTestbase/nsk/{jdi,jdw,jvmti}, serviceability/{jdwp,jvmti} . 
> > > > 
> > > > Thanks,
> > > > 
> > > > Yasumasa
> > > 
> 


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