RFR: 8242427: JVMTI frame pop operations should use Thread-Local Handshakes
Patricio Chilano
patricio.chilano.mateo at oracle.com
Thu Aug 27 06:20:06 UTC 2020
Hi Yasumasa,
On 8/26/20 8:57 PM, Yasumasa Suenaga wrote:
> Hi Patricio,
>
> Thanks for your review, but webrev.00 has been rotten.
> Can you review webrev.02?
>
> webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.02/
> diff between webrev.00 and webrev.01:
> http://hg.openjdk.java.net/jdk/submit/rev/7facd1dd39d6
> diff between webrev.01 and webrev.02:
> http://hg.openjdk.java.net/jdk/submit/rev/2ef7feb5681f
Looks good to me. Can JvmtiEventController::set_frame_pop(),
JvmtiEventController::clear_frame_pop() and
JvmtiEventController::clear_to_frame_pop() still be called at a safepoint?
Thanks,
Patricio
> Thanks,
>
> Yasumasa
>
>
> On 2020/08/27 7:50, Patricio Chilano wrote:
>> Hi Yasumasa,
>>
>> On 8/26/20 4:34 AM, Yasumasa Suenaga wrote:
>>> Hi Patricio, David,
>>>
>>> Thanks for your comment!
>>>
>>> I updated webrev which includes the fix which is commented by
>>> Patricio, and it passed submit repo. So I switch this mail thread to
>>> RFR.
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8242427
>>> webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.00/
>> The changes look good to me, thanks for fixing them.
>>
>> Patricio
>>> I understand David said same concerns as Patricio about active
>>> handshaker. This webrev checks active handshaker is current thread
>>> or not.
>>>
>>>
>>> Cheers,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/08/26 10:13, Patricio Chilano wrote:
>>>> Hi Yasumasa,
>>>>
>>>> On 8/23/20 11:40 PM, Yasumasa Suenaga wrote:
>>>>> Hi all,
>>>>>
>>>>> I want to hear your opinions about the change for JDK-8242427.
>>>>>
>>>>> I'm trying to migrate following operations to direct handshake.
>>>>>
>>>>> - VM_UpdateForPopTopFrame
>>>>> - VM_SetFramePop
>>>>> - VM_GetCurrentLocation
>>>>>
>>>>> Some operations (VM_GetCurrentLocation and
>>>>> EnterInterpOnlyModeClosure) might be called at safepoint, so I
>>>>> want to use JavaThread::active_handshaker() in production VM to
>>>>> detect the process is in direct handshake or not.
>>>>>
>>>>> However this function is available in debug VM only, so I want to
>>>>> hear the reason why it is for debug VM only, and there are no
>>>>> problem to use it in production VM. Of course another solutions
>>>>> are welcome.
>>>> I added the _active_handshaker field to the HandshakeState class
>>>> when working on 8230594 to adjust some asserts, where instead of
>>>> checking for the VMThread we needed to check for the active
>>>> handshaker of the target JavaThread. Since there were no other
>>>> users of it, there was no point in declaring it and having to write
>>>> to it for the release bits. There are no issues with having it in
>>>> production though so you could change that if necessary.
>>>>
>>>>> webrev is here. It passed jtreg tests
>>>>> (vmTestbase/nsk/{jdi,jdwp,jvmti} serviceability/{jdwp,jvmti})
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/proposal/
>>>> Some comments on the proposed change.
>>>>
>>>> src/hotspot/share/prims/jvmtiEnvThreadState.cpp,
>>>> src/hotspot/share/prims/jvmtiEventController.cpp
>>>> Why is the check to decide whether to call the handshake or execute
>>>> the operation with the current thread different for
>>>> GetCurrentLocationClosure vs EnterInterpOnlyModeClosure?
>>>>
>>>> (GetCurrentLocationClosure)
>>>> if ((Thread::current() == _thread) || (_thread->active_handshaker()
>>>> != NULL)) {
>>>> op.do_thread(_thread);
>>>> } else {
>>>> Handshake::execute_direct(&op, _thread);
>>>> }
>>>>
>>>> vs
>>>>
>>>> (EnterInterpOnlyModeClosure)
>>>> if (target->active_handshaker() != NULL) {
>>>> hs.do_thread(target);
>>>> } else {
>>>> Handshake::execute_direct(&hs, target);
>>>> }
>>>>
>>>> If you change VM_SetFramePop to use handshakes then it seems you
>>>> could reach JvmtiEventControllerPrivate::enter_interp_only_mode()
>>>> with the current thread being the target.
>>>> Also I think you want the second expression of that check to be
>>>> (target->active_handshaker() == Thread::current()). So either you
>>>> are the target or the current active_handshaker for that target.
>>>> Otherwise active_handshaker() could be not NULL because there is
>>>> another JavaThread handshaking the same target. Unless you are
>>>> certain that it can never happen, so if active_handshaker() is not
>>>> NULL it is always the current thread, but even in that case this
>>>> way is safer.
>>>>
>>>> src/hotspot/share/prims/jvmtiThreadState.cpp
>>>> The guarantee() statement exists in release builds too so the
>>>> "#ifdef ASSERT" directive should be removed, otherwise "current"
>>>> will not be declared.
>>>>
>>>> Thanks!
>>>>
>>>> Patricio
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>
>>
More information about the serviceability-dev
mailing list