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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Fri Sep 4 10:29:18 UTC 2020


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