RFR: 8248362: JVMTI frame operations should use Thread-Local Handshake

[Daniel D. Daugherty]

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.

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...

     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.

src/hotspot/share/prims/jvmtiEnvBase.hpp
     L580: // HandshakeClosure to frame location.
         typo - s/to frame/to get frame/

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.

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