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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Sat Sep 5 00:36:52 UTC 2020


Hi Yasumasa,

I'm okay with the fix.

Thanks,
Serguei


On 9/4/20 03:52, Yasumasa Suenaga wrote:
> Hi Serguei,
>
> Thank you for the explanation.
> I agree with you it is hard to prove that it is safe to get rid of 
> these handshake closures.
>
> So are you ok with webrev.04 (latest webrev) for this enhancement?
> I know you give +1 to this change, but I want to confirm in light of 
> the discussion again so far.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2020/09/04 19:29, serguei.spitsyn at oracle.com wrote:
>> Hi Yasumasa,
>>
>>
>> On 9/4/20 02:49, Yasumasa Suenaga wrote:
>>> Hi Serguei,
>>>
>>> For example, JvmtiThreadState_lock would be grabbed at 
>>> ciEnv::cache_jvmti_state() which is called by 
>>> CompileBroker::invoke_compiler_on_method(). Compiler thread might be 
>>> touch the protected resource even if target thread is suspended with 
>>> direct handshake.
>>
>> My statement was: "Grabbing the JvmtiThreadState_lock is good enough 
>> to ensure MT-safety".
>> Compiler code just reads but does not change the thread state.
>> All modifications of the thread state are protected with the 
>> JvmtiThreadState_lock.
>> In all cases when the state is changed by non-current thread the 
>> target thread is suspended (please, let me know if it is a wrong 
>> statement).
>> So, it looks like these two handshake closures are not needed: 
>> UpdateForPopTopFrameClosure and SetFramePopClosure.
>> However, it is hard to prove that it is safe to get rid of these 
>> handshake closures.
>>
>> Thanks,
>> Serguei
>>>
>>> So I think they are needed both direct handshake and 
>>> JvmtiThreadState_lock.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/09/04 17:38, serguei.spitsyn at oracle.com wrote:
>>>> Hi Yasumasa,
>>>>
>>>> Only PopFrame, ForceEarlyReturn, NotifyFramePop and ResumeThread 
>>>> require the target thread to be suspended.
>>>> Other JVMTI functions do not require it.
>>>>
>>>> I'm still thinking if it is a good idea to get rid of the 
>>>> UpdateForPopTopFrameClosure and SetFramePopClosure hadnshakes.
>>>> It has to be safe because the target thread is suspended.
>>>> Grabbing the JvmtiThreadState_lock is good enough to ensure MT-safety.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 9/4/20 01:28, Yasumasa Suenaga wrote:
>>>>> Hi Serguei,
>>>>>
>>>>> Thanks for your review!
>>>>>
>>>>> On 2020/09/04 15:43, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Yasumasa,
>>>>>>
>>>>>> Thanks for the answer and sorry for the latency. It is not easy 
>>>>>> to double check everything related to the JvmtiThreadState_lock use.
>>>>>> You are right, it is not safe to remove the MutexLockers around 
>>>>>> the direct handshakes (unfortunately, I'm still floating with the 
>>>>>> handshake closures).
>>>>>>
>>>>>> I also had this question:
>>>>>> Q: Why do we need UpdateForPopTopFrameClosure and 
>>>>>> SetFramePopClosure if we already protect data with the 
>>>>>> JvmtiThreadState_lock?
>>>>>
>>>>> We need to suspend target thread (e.g. debugee) if we want to do 
>>>>> frame operation. I think we cannot do frame operation safety 
>>>>> without stopping target thread.
>>>>>
>>>>> So I chose direct handshake to avoid stopping the other threads.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>>> However, I'd be even morenervous to get rid of these closures as 
>>>>>> it is hard to predict all the consequences. The concern is the 
>>>>>> JvmtiEnvThreadState functions like get_frame_pops, has_frame_pos 
>>>>>> and is_frame_pop. They were designed to be called on the current 
>>>>>> thread or in a VMops/Handshake. Changing the _frame_pops on other 
>>>>>> threads without handshakes (even under protection of the 
>>>>>> JvmtiThreadState_lock) will break this design as these functions 
>>>>>> are not protected with the JvmtiThreadState_lock.
>>>>>>
>>>>>> So, I'm getting along with your sync approach now.
>>>>>> There is a similar sync pattern with the 
>>>>>> EnterInterpOnlyModeClosure and functions 
>>>>>> enter_interp_only_mode/leave_interp_only_mode which are called in 
>>>>>> the recompute_thread_enabled and recompute_enabled. The 
>>>>>> JvmtiThreadState_lock is also grabbed around this handshake closure.
>>>>>>
>>>>>> You fix moved MutexLockers from the JvmtiEventController 
>>>>>> functions set_frame_pop and clear_frame_pop to all their callers 
>>>>>> which should be fine in general. The function 
>>>>>> JvmtiEventController::clear_to_frame_pop is used by the 
>>>>>> JvmtiEnvThreadState::clear_to_frame_pop which in turn not used 
>>>>>> itself.
>>>>>>
>>>>>> So, I'm okay with the fix.
>>>>>> Thank you for your patience and answers!
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>> On 9/3/20 02:01, Yasumasa Suenaga wrote:
>>>>>>> Hi Serguei,
>>>>>>>
>>>>>>> Your suggestion would not grab lock if it performs in direct 
>>>>>>> handshake, then other threads can enter these functions. Is it 
>>>>>>> safe?
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>> On 2020/09/03 16:34, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Hi Yasumasa,
>>>>>>>>
>>>>>>>> Wood it be MT-safe to do this to avoid locks at handshake time? :
>>>>>>>>
>>>>>>>>   JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, 
>>>>>>>> JvmtiFramePop fpop) {
>>>>>>>> -  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? 
>>>>>>>> NULL : JvmtiThreadState_lock);
>>>>>>>> +  MutexLocker mu(Thread::current == 
>>>>>>>> ets->get_thread()->active_handshaker() ? NULL : 
>>>>>>>> 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);
>>>>>>>> +  MutexLocker mu(Thread::current == 
>>>>>>>> ets->get_thread()->active_handshaker() ? NULL : 
>>>>>>>> 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);
>>>>>>>> +  MutexLocker mu(Thread::current == 
>>>>>>>> ets->get_thread()->active_handshaker() ? NULL : 
>>>>>>>> JvmtiThreadState_lock);
>>>>>>>> JvmtiEventControllerPrivate::clear_to_frame_pop(ets, fpop);
>>>>>>>>   }
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/2/20 01:42, serguei.spitsyn at oracle.com wrote:
>>>>>>>>> Hi Yasumasa,
>>>>>>>>>
>>>>>>>>> Thank you for the explanation. At least, I see the real 
>>>>>>>>> motivation.
>>>>>>>>> I've overlooked this in the email thread.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Serguei
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 9/2/20 00:00, Yasumasa Suenaga wrote:
>>>>>>>>>> Hi Serguei,
>>>>>>>>>>
>>>>>>>>>> On 2020/09/02 15: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.
>>>>>>>>>>
>>>>>>>>>> In addition we cannot take the MutexLocker inside a 
>>>>>>>>>> handshakes because we might see deadlock with the handshake 
>>>>>>>>>> semaphore.
>>>>>>>>>> So we have to grab JvmtiThreadState_lock before 
>>>>>>>>>> execute_direct().
>>>>>>>>>>
>>>>>>>>>> https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-August/032754.html 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> 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