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

[Daniel D. Daugherty]

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