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

[Yasumasa Suenaga]

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