RFR: 8242427: JVMTI frame pop operations should use Thread-Local Handshakes

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Wed Sep 2 07:10:18 UTC 2020


Hi David,


On 9/1/20 23:29, David Holmes wrote:
> Hi Serguei,
>
> On 2/09/2020 4:11 pm, serguei.spitsyn at oracle.com wrote:
>> Hi Yasumasa,
>>
>> It seems to me your update for sync with the JvmtiThreadState_lock is 
>> incorrect.
>> Let me explain it.
>> The original design was that the functions is_frame_pop, 
>> set_frame_pop, clear_frame_pop and clear_to_frame_pop are always 
>> called either on the current thread or in a VMop.
>> There are 3 levels of these functions: in JvmtiEnvThreadState, 
>> JvmtiEventController and JvmtiEventControllerPrivate.
>> You already found the JvmtiThreadState_lock is grabbed in the 
>> JvmtiEventController versions of these functions.
>> It is for MT-safety of the recompute_thread_enabled() which can be 
>> called not only on current thread and VMop.
>
> Right, but now that we use a handshake, not a VMop, we have no 
> safepoint to guarantee MT-safety and so we have to use the lock to 
> ensure that.

Thank you for the comment.
My understanding is that a handshake (at least, direct) is an equivalent 
of the current thread.
Is it correct?

Thanks,
Serguei

>
> David
> -----
>
>> So, I think adding MutexLocker's to the jvmtiEnv.cpp and 
>> jvmtiExport.cpp is not needed:
>>
>> +      MutexLocker mu(JvmtiThreadState_lock);
>> +      if (java_thread == JavaThread::current()) {
>> +        state->update_for_pop_top_frame();
>> +      } else {
>> +        UpdateForPopTopFrameClosure op(state);
>>
>> . . .
>>
>> +  MutexLocker mu(JvmtiThreadState_lock);
>>     if (java_thread == JavaThread::current()) {
>>       int frame_number = state->count_frames() - depth;
>> state->env_thread_state(this)->set_frame_pop(frame_number);
>>     } else {
>> -    VM_SetFramePop op(this, state, depth);
>> -    VMThread::execute(&op);
>> -    err = op.result();
>> +    SetFramePopClosure op(this, state, depth);
>> +    bool executed = Handshake::execute_direct(&op, java_thread);
>> +    err = executed ? op.result() : JVMTI_ERROR_THREAD_NOT_ALIVE;
>>
>> . . .
>>
>> + MutexLocker mu(JvmtiThreadState_lock);
>>           ets->clear_frame_pop(cur_frame_number);
>>
>>
>> Instead, they have to be restored in the JvmtiEventController functions:
>>
>>   void
>>   JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, 
>> JvmtiFramePop fpop) {
>> -  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : 
>> JvmtiThreadState_lock);
>> +  assert_lock_strong(JvmtiThreadState_lock);
>>     JvmtiEventControllerPrivate::set_frame_pop(ets, fpop);
>>   }
>>       void
>>   JvmtiEventController::clear_frame_pop(JvmtiEnvThreadState *ets, 
>> JvmtiFramePop fpop) {
>> -  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : 
>> JvmtiThreadState_lock);
>> +  assert_lock_strong(JvmtiThreadState_lock);
>>     JvmtiEventControllerPrivate::clear_frame_pop(ets, fpop);
>>   }
>>       void
>>   JvmtiEventController::clear_to_frame_pop(JvmtiEnvThreadState *ets, 
>> JvmtiFramePop fpop) {
>> -  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : 
>> JvmtiThreadState_lock);
>> +  assert_lock_strong(JvmtiThreadState_lock);
>>     JvmtiEventControllerPrivate::clear_to_frame_pop(ets, fpop);
>>   }
>>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 9/1/20 21:34, serguei.spitsyn at oracle.com wrote:
>>> Hi Yasumasa,
>>>
>>>
>>> On 9/1/20 21:17, Yasumasa Suenaga wrote:
>>>> Hi David,
>>>>
>>>> On 2020/09/02 13:13, David Holmes wrote:
>>>>> Hi Yasumasa,
>>>>>
>>>>> On 31/08/2020 7:10 pm, Yasumasa Suenaga wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> I uploaded new webrev. Could you review again?
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.04/
>>>>>>
>>>>>> This webrev includes two changes:
>>>>>>
>>>>>>    1. Use assert_lock_strong() for JvmtiThreadState_lock
>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/c85f93d2042d
>>>>>>
>>>>>>    2. Check return value from execute_direct() with assert()
>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/8746e1651343
>>>>>
>>>>> The message for the assertion:
>>>>>
>>>>> assert(executed, "Direct handshake failed. Target thread is still 
>>>>> alive?");
>>>>>
>>>>> should be phrased:
>>>>>
>>>>> assert(executed, "Direct handshake failed. Target thread is not 
>>>>> alive?");
>>>>>
>>>>> otherwise it sounds like the expectation is that it should not be 
>>>>> alive.
>>>>>
>>>>> Other changes fine.
>>>>>
>>>>> No need to see updated webrev.
>>>>
>>>> Thanks for your review!
>>>> I will fix them before pushing.
>>>
>>> Please, hold on.
>>> I'm still reviewing this.
>>> It is not clear yet if sync with the JvmtiThreadState_lock is fully 
>>> correct.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 2020/08/31 15:22, Yasumasa Suenaga wrote:
>>>>>>> Hi David,
>>>>>>>
>>>>>>> On 2020/08/31 14:43, David Holmes wrote:
>>>>>>>> Hi Yasumasa,
>>>>>>>>
>>>>>>>> On 28/08/2020 1:01 pm, Yasumasa Suenaga wrote:
>>>>>>>>> Hi David,
>>>>>>>>>
>>>>>>>>> On 2020/08/28 11:04, David Holmes wrote:
>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>
>>>>>>>>>> On 28/08/2020 11:24 am, Yasumasa Suenaga wrote:
>>>>>>>>>>> Hi David,
>>>>>>>>>>>
>>>>>>>>>>> On 2020/08/27 15:49, David Holmes wrote:
>>>>>>>>>>>> Sorry I just realized I reviewed version 00 :(
>>>>>>>>>>
>>>>>>>>>> Note that my comments on version 00 in my earlier email still 
>>>>>>>>>> apply.
>>>>>>>>>
>>>>>>>>> I copied here your comment on webrev.00:
>>>>>>>>>
>>>>>>>>>>>>> I see. It is a pity that we have now lost that critical 
>>>>>>>>>>>>> indicator that shows how this operation can be nested 
>>>>>>>>>>>>> within another operation. The possibility of nesting is 
>>>>>>>>>>>>> even more obscure with 
>>>>>>>>>>>>> JvmtiEnvThreadState::reset_current_location. And the fact 
>>>>>>>>>>>>> it is now up to the caller to handle that case explicitly 
>>>>>>>>>>>>> raises some concern - what will happen if you call 
>>>>>>>>>>>>> execute_direct whilst already in a handshake with the 
>>>>>>>>>>>>> target thread?
>>>>>>>>>
>>>>>>>>> I heard deadlock would be happen if execute_direct() calls in 
>>>>>>>>> direct handshake. Thus we need to use active_handshaker() in 
>>>>>>>>> this change.
>>>>>>>>
>>>>>>>> Okay. This is something we need to clarify with direct 
>>>>>>>> handshake usage information. I think it would be preferable if 
>>>>>>>> this was handled in execute_direct rather than the caller ... 
>>>>>>>> though it may also be the case that we need the writer of the 
>>>>>>>> handshake operation to give due consideration to nesting ...
>>>>>>>
>>>>>>> Agree, I also prefer to check whether caller is in direct 
>>>>>>> handshake in execute_direct().
>>>>>>> But I think this is another enhancement because we need to 
>>>>>>> change the behavior of execute_direct().
>>>>>>>
>>>>>>>
>>>>>>>>>>>>> Further comments:
>>>>>>>>>>>>>
>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvThreadState.cpp
>>>>>>>>>>>>>
>>>>>>>>>>>>>   194 #ifdef ASSERT
>>>>>>>>>>>>>   195   Thread *current = Thread::current();
>>>>>>>>>>>>>   196 #endif
>>>>>>>>>>>>>   197   assert(get_thread() == current || current == 
>>>>>>>>>>>>> get_thread()->active_handshaker(),
>>>>>>>>>>>>>   198          "frame pop data only accessible from same 
>>>>>>>>>>>>> thread or direct handshake");
>>>>>>>>>>>>>
>>>>>>>>>>>>> Can you factor this out into a separate function so that 
>>>>>>>>>>>>> it is not repeated so often. Seems to me that there should 
>>>>>>>>>>>>> be a global function on Thread: 
>>>>>>>>>>>>> assert_current_thread_or_handshaker() [yes unpleasant name 
>>>>>>>>>>>>> but ...] that will allow us to stop repeating this code 
>>>>>>>>>>>>> fragment across numerous files. A follow up RFE for that 
>>>>>>>>>>>>> would be okay too (I see some guarantees that should 
>>>>>>>>>>>>> probably just be asserts so they need a bit more checking).
>>>>>>>>>
>>>>>>>>> I filed it as another RFE:
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8252479
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>>>   331 Handshake::execute_direct(&op, _thread);
>>>>>>>>>>>>>
>>>>>>>>>>>>> You aren't checking the return value of execute_direct, 
>>>>>>>>>>>>> but I can't tell where _thread was checked for still being 
>>>>>>>>>>>>> alive ??
>>>>>>>>>>>>>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEventController.cpp
>>>>>>>>>>>>>
>>>>>>>>>>>>>   340 Handshake::execute_direct(&hs, target);
>>>>>>>>>>>>>
>>>>>>>>>>>>> I know this is existing code but I have the same query as 
>>>>>>>>>>>>> above - no return value check and no clear check that the 
>>>>>>>>>>>>> JavaThread is still alive?
>>>>>>>>>
>>>>>>>>> Existing code seems to assume that target thread is alive, 
>>>>>>>>> frame operations (e.g. PopFrame()) should be performed on live 
>>>>>>>>> thread. And also existing code would not set any JVMTI error 
>>>>>>>>> and cannot propagate it to caller. So I do not add the check 
>>>>>>>>> for thread state.
>>>>>>>>
>>>>>>>> Okay. But note that for PopFrame the tests for isAlive and 
>>>>>>>> is-suspended have already been performed before we do the 
>>>>>>>> execute_direct; so in that case we could simply assert that 
>>>>>>>> execute_direct returns true. Similarly for other cases.
>>>>>>>
>>>>>>> Ok, I will change as following in next webrev:
>>>>>>>
>>>>>>> ```
>>>>>>> bool result = Handshake::execute_direct(&hs, target);
>>>>>>> guarantee(result, "Direct handshake failed. Target thread is 
>>>>>>> still alive?");
>>>>>>> ```
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>>>>>>>> Do we know if the existing tests actually test the nested 
>>>>>>>>>>>>> cases?
>>>>>>>>>
>>>>>>>>> I saw some error with assertion for JvmtiThreadState_lock and 
>>>>>>>>> safepoint in vmTestbase at first, so I guess nested call would 
>>>>>>>>> be tested, but I'm not sure.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>> I have concerns with the added locking:
>>>>>>>>>>>>
>>>>>>>>>>>> MutexLocker mu(JvmtiThreadState_lock);
>>>>>>>>>>>>
>>>>>>>>>>>> Who else may be holding that lock? Could it be our target 
>>>>>>>>>>>> thread that we have already initiated a handshake with? 
>>>>>>>>>>>> (The lock ranking checks related to safepoints don't help 
>>>>>>>>>>>> us detect deadlocks between a target thread and its 
>>>>>>>>>>>> handshaker. :( )
>>>>>>>>>>>
>>>>>>>>>>> I checked source code again, then I couldn't find the point 
>>>>>>>>>>> that target thread already locked JvmtiThreadState_lock at 
>>>>>>>>>>> direct handshake.
>>>>>>>>>>
>>>>>>>>>> I'm very unclear exactly what state this lock guards and 
>>>>>>>>>> under what conditions. But looking at:
>>>>>>>>>>
>>>>>>>>>> src/hotspot/share/prims/jvmtiEnv.cpp
>>>>>>>>>>
>>>>>>>>>> Surely the lock is only needed in the direct-handshake case 
>>>>>>>>>> and not when operating on the current thread? Or is it there 
>>>>>>>>>> because you've removed the locking from the lower-level 
>>>>>>>>>> JvmtiEventController methods and so now you need to take the 
>>>>>>>>>> lock higher-up the call chain? (I find it hard to follow the 
>>>>>>>>>> call chains in the JVMTI code.)
>>>>>>>>>
>>>>>>>>> We need to take the lock higher-up the call chain. It is 
>>>>>>>>> suggested by Robbin, and works fine.
>>>>>>>>
>>>>>>>> Okay. It seems reasonably safe in this context as there is 
>>>>>>>> little additional work done while holding the lock.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>> It is far from clear now which functions are reachable from 
>>>>>>>>>>>> handshakes, which from safepoint VM_ops and which from both.
>>>>>>>>>>>>
>>>>>>>>>>>> ! assert(SafepointSynchronize::is_at_safepoint() || 
>>>>>>>>>>>> JvmtiThreadState_lock->is_locked(), "Safepoint or must be 
>>>>>>>>>>>> locked");
>>>>>>>>>>>>
>>>>>>>>>>>> This can be written as:
>>>>>>>>>>>>
>>>>>>>>>>>> assert_locked_or_safepoint(JvmtiThreadState_lock);
>>>>>>>>>>>>
>>>>>>>>>>>> or possibly the weak variant of that. ('m puzzled by the 
>>>>>>>>>>>> extra check in the strong version ... I think it is 
>>>>>>>>>>>> intended for the case of the VMThread executing a 
>>>>>>>>>>>> non-safepoint VMop.)
>>>>>>>>>>>
>>>>>>>>>>>> JvmtiEventController::set_frame_pop(), 
>>>>>>>>>>>> JvmtiEventController::clear_frame_pop() and 
>>>>>>>>>>>> JvmtiEventController::clear_to_frame_pop() are no longer 
>>>>>>>>>>>> called at safepoint, so I remove safepoint check from 
>>>>>>>>>>>> assert() in new webrev.
>>>>>>>>>>
>>>>>>>>>> You should use assert_lock_strong for this.
>>>>>>>>>
>>>>>>>>> I will do that.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> David
>>>>>>>>>>
>>>>>>>>>>>    webrev: 
>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.03/
>>>>>>>>>>>      diff from previous webrev: 
>>>>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/2a2c02ada080
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> Yasumasa
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> David
>>>>>>>>>>>> -----
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 27/08/2020 4:34 pm, David Holmes wrote:
>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 27/08/2020 9:40 am, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 2020/08/27 8:09, David Holmes wrote:
>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 26/08/2020 5:34 pm, 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/ 
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I understand David said same concerns as Patricio about 
>>>>>>>>>>>>>>>> active handshaker. This webrev checks active handshaker 
>>>>>>>>>>>>>>>> is current thread or not.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> How can the current thread already be in a handshake 
>>>>>>>>>>>>>>> with the target when you execute this code?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> EnterInterpOnlyModeClosure might be called in handshake 
>>>>>>>>>>>>>> with UpdateForPopTopFrameClosure or with SetFramePopClosure.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> EnterInterpOnlyModeClosure is introduced in JDK-8238585 
>>>>>>>>>>>>>> as an alternative in VM_EnterInterpOnlyMode.
>>>>>>>>>>>>>> VM_EnterInterpOnlyMode returned true in 
>>>>>>>>>>>>>> allow_nested_vm_operations(). Originally, it could have 
>>>>>>>>>>>>>> been called from other VM operations.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I see. It is a pity that we have now lost that critical 
>>>>>>>>>>>>> indicator that shows how this operation can be nested 
>>>>>>>>>>>>> within another operation. The possibility of nesting is 
>>>>>>>>>>>>> even more obscure with 
>>>>>>>>>>>>> JvmtiEnvThreadState::reset_current_location. And the fact 
>>>>>>>>>>>>> it is now up to the caller to handle that case explicitly 
>>>>>>>>>>>>> raises some concern - what will happen if you call 
>>>>>>>>>>>>> execute_direct whilst already in a handshake with the 
>>>>>>>>>>>>> target thread?
>>>>>>>>>>>>>
>>>>>>>>>>>>> I can't help but feel that we need a more rigorous and 
>>>>>>>>>>>>> automated way of dealing with nesting ... perhaps we don't 
>>>>>>>>>>>>> even need to care and handshakes should always allow 
>>>>>>>>>>>>> nested handshake requests? (Question more for Robbin and 
>>>>>>>>>>>>> Patricio.)
>>>>>>>>>>>>>
>>>>>>>>>>>>> Further comments:
>>>>>>>>>>>>>
>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvThreadState.cpp
>>>>>>>>>>>>>
>>>>>>>>>>>>>   194 #ifdef ASSERT
>>>>>>>>>>>>>   195   Thread *current = Thread::current();
>>>>>>>>>>>>>   196 #endif
>>>>>>>>>>>>>   197   assert(get_thread() == current || current == 
>>>>>>>>>>>>> get_thread()->active_handshaker(),
>>>>>>>>>>>>>   198          "frame pop data only accessible from same 
>>>>>>>>>>>>> thread or direct handshake");
>>>>>>>>>>>>>
>>>>>>>>>>>>> Can you factor this out into a separate function so that 
>>>>>>>>>>>>> it is not repeated so often. Seems to me that there should 
>>>>>>>>>>>>> be a global function on Thread: 
>>>>>>>>>>>>> assert_current_thread_or_handshaker() [yes unpleasant name 
>>>>>>>>>>>>> but ...] that will allow us to stop repeating this code 
>>>>>>>>>>>>> fragment across numerous files. A follow up RFE for that 
>>>>>>>>>>>>> would be okay too (I see some guarantees that should 
>>>>>>>>>>>>> probably just be asserts so they need a bit more checking).
>>>>>>>>>>>>>
>>>>>>>>>>>>>   331 Handshake::execute_direct(&op, _thread);
>>>>>>>>>>>>>
>>>>>>>>>>>>> You aren't checking the return value of execute_direct, 
>>>>>>>>>>>>> but I can't tell where _thread was checked for still being 
>>>>>>>>>>>>> alive ??
>>>>>>>>>>>>>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEventController.cpp
>>>>>>>>>>>>>
>>>>>>>>>>>>>   340 Handshake::execute_direct(&hs, target);
>>>>>>>>>>>>>
>>>>>>>>>>>>> I know this is existing code but I have the same query as 
>>>>>>>>>>>>> above - no return value check and no clear check that the 
>>>>>>>>>>>>> JavaThread is still alive?
>>>>>>>>>>>>>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>> Do we know if the existing tests actually test the nested 
>>>>>>>>>>>>> cases?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> David
>>>>>>>>>>>>> -----
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 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