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

[David Holmes]

Hi Dan,

I just want to respond to one aspect of your response ...

On 23/07/2020 11:04 am, 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.
> 
> 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.

I understand why, when looking at this change, you would like the 
comment, but outside of the webrev the reference to ThreadsListHandle 
doesn't really have any context. We need a TLH anywhere there is a risk 
the target thread might exit while we're interacting with it, but that 
can't happen in the context of a direct handshake operation with the 
target thread because the TLH exists in our caller. This is the new 
normal for code that executes as part of a direct handshake.

David
-----

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