3-nd round RFR 6471769: Error: assert(_cur_stack_depth == count_frames(), "cur_stack_depth out of sync")
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Feb 28 13:26:43 PST 2014
On 2/28/14 2:24 PM, serguei.spitsyn at oracle.com wrote:
> On 2/28/14 1:12 PM, Daniel D. Daugherty wrote:
>> On 2/28/14 12:55 PM, serguei.spitsyn at oracle.com wrote:
>>> On 2/27/14 10:04 PM, David Holmes wrote:
>>>> Hi Serguei,
>>>>
>>>> On 28/02/2014 1:50 PM, serguei.spitsyn at oracle.com wrote:
>>>>> Please, review the fix for:
>>>>> https://bugs.openjdk.java.net/browse/JDK-6471769
>>>>>
>>>>>
>>>>> Open webrev:
>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/6471769-JVMTI-DEPTH.3
>>>>>
>>
>> Thumbs up! (including the tweaks in the .4 version)
>
> Thanks a lot, Dan!
>
>>
>> src/share/vm/runtime/vm_operations.hpp
>> No comments.
>>
>> src/share/vm/prims/jvmtiEnvBase.hpp
>> line 375: bool allow_nested_vm_operations() const { return
>> true; }
>> Does VM_SetFramePop really gave to permit nested VMops?
>
> Yes.
> It was a real surprise that it is really necessary.
Maybe add a one line comment saying something like:
// needs to permit nested because this VMop can be invoked from XXX VMop...
or something like that.
Dan
>
> Thanks!
> Serguei
>
>>
>> src/share/vm/prims/jvmtiEnv.cpp
>> No comments.
>>
>> src/share/vm/prims/jvmtiEnvThreadState.cpp
>> No comments.
>>
>> src/share/vm/prims/jvmtiEventController.cpp
>> No comments.
>>
>> src/share/vm/prims/jvmtiThreadState.cpp
>> No comments.
>>
>> Dan
>>
>>
>>
>>>>>
>>>>>
>>>>> Summary:
>>>>>
>>>>> It is another attempt to fix the JTREG com/sun/jdi tests
>>>>> regression
>>>>> discovered in the first round change.
>>>>> The fix is to avoid lock synchronization at
>>>>> safepoints(jvmtiEventController.cpp).
>>>>> Thanks to Dan for catching the problem in the 2-nd round of
>>>>> review!
>>>>
>>>> The basic approach here seems sound.
>>>
>>> Thank you for reviewing the fix!
>>>
>>>>
>>>> I find the checking for cur->is_VMThread() somewhat overly
>>>> conservative - if we are at a safepoint, and executing this code,
>>>> then we must be the VMThread. But ok.
>>>
>>> Agreed and simplified. Thanks!
>>>
>>>>
>>>> You could also use MutexLockerEx to avoid the need for locked and
>>>> unlocked paths to a common call, but that's just stylistic. Though
>>>> if you are grabbing the current thread anyway you can also use the
>>>> MutexLocker calls that take the thread arg - to avoid a second
>>>> look-up of the current thread.
>>>
>>> Thank you for reminding. I keep forgetting about it.
>>> Will check what is better here, just do not want to rerun the whole
>>> testing.
>>> But I'm in favor to make it simpler. :)
>>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>> David
>>>> -----
>>>>
>>>>> Testing:
>>>>> All tests are passed: nsk.jvmti, nsk.jdi, nsk.jdwp, JTreg
>>>>> com/sun/jdi
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 2/27/14 2:00 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> On 2/27/14 1:03 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>> On 2/27/14 12:28 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Dan,
>>>>>>>>
>>>>>>>> Thank you a lot for reviewing this!
>>>>>>>>
>>>>>>>> On 2/27/14 11:09 AM, Daniel D. Daugherty wrote:
>>>>>>>>> On 2/27/14 1:25 AM, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>> Please, review the fix for:
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-6471769
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Open webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/6471769-JVMTI-DEPTH.2
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> src/share/vm/runtime/vm_operations.hpp
>>>>>>>>> No comments.
>>>>>>>>>
>>>>>>>>> src/share/vm/prims/jvmtiEnvBase.hpp
>>>>>>>>> No comments.
>>>>>>>>>
>>>>>>>>> src/share/vm/prims/jvmtiEnv.cpp
>>>>>>>>> No comments.
>>>>>>>>>
>>>>>>>>> src/share/vm/prims/jvmtiEnvThreadState.cpp
>>>>>>>>> No comments.
>>>>>>>>>
>>>>>>>>> src/share/vm/prims/jvmtiEventController.cpp
>>>>>>>>> JvmtiEventController::set_frame_pop() is called by
>>>>>>>>> JvmtiEnvThreadState::set_frame_pop() which is called by
>>>>>>>>> JvmtiEnv::NotifyFramePop().
>>>>>>>>>
>>>>>>>>> The "MutexLocker mu(JvmtiThreadState_lock)" in
>>>>>>>>> JvmtiEventController::set_frame_pop() protected the work
>>>>>>>>> done by JvmtiEventControllerPrivate::set_frame_pop():
>>>>>>>>>
>>>>>>>>> ets->get_frame_pops()->set(fpop);
>>>>>>>>> recompute_thread_enabled(ets->get_thread()->jvmti_thread_state());
>>>>>>>>>
>>>>>>>>
>>>>>>>> Your check is the right thing to do, thanks!
>>>>>>>> I had to explain this more clearly in this 2-nd review request.
>>>>>>>>
>>>>>>>> The approach I've taken here is that all this code paths are
>>>>>>>> executed
>>>>>>>> on the target thread or at a safepoint.
>>>>>>>>
>>>>>>>> It is true for all 3 functions:
>>>>>>>> set_frame_pop(), clear_frame_pop() and clear_to_frame_pop().
>>>>>>>>
>>>>>>>> And the updated assert guards ensure that it is the case.
>>>>>>>>
>>>>>>>> It could be a good idea to add a No_Safepoint_Verifier for
>>>>>>>> PopFrame() and NotifyFramePop()
>>>>>>>> to make sure the current/target thread does not go to safepoint
>>>>>>>> until it is returned from
>>>>>>>> update_for_pop_top_frame() and set_frame_pop() correspondingly.
>>>>>>>> A No_Safepoint_Verifier can be also needed in the
>>>>>>>> JvmtiExport::post_method_exit().
>>>>>>>>
>>>>>>>> These are all places where these functions are called:
>>>>>>>> prims/jvmtiEnv.cpp:
>>>>>>>> state->env_thread_state(this)->set_frame_pop(frame_number); //
>>>>>>>> JvmtiEnv::NotifyFramePop()
>>>>>>>> prims/jvmtiExport.cpp: ets->clear_frame_pop(cur_frame_number); //
>>>>>>>> JvmtiExport::post_method_exit()
>>>>>>>> prims/jvmtiThreadState.cpp:
>>>>>>>> ets->clear_frame_pop(popframe_number); //
>>>>>>>> JvmtiThreadState::update_for_pop_top_frame()
>>>>>>>>
>>>>>>>> The function JvmtiEnvThreadState::clear_to_frame_pop() is never
>>>>>>>> called now.
>>>>>>>
>>>>>>> There is still a concern about recompute_thread_enabled().
>>>>>>> If it is normally always protected with the JvmtiThreadState_lock
>>>>>>> then the approach above is not going to work.
>>>>>>> I'm trying to check this now.
>>>>>>
>>>>>> Dan,
>>>>>>
>>>>>> I came to a conclusion that these 3 functions still must be
>>>>>> protected
>>>>>> by the JvmtiThreadState_lock when they are called out of a
>>>>>> safepoint.
>>>>>> It is a little bit ugly but has to be safe though.
>>>>>>
>>>>>> Please, let me know if you see eny problems with that.
>>>>>> I'll send a new webrev soon.
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Since multiple threads can call JVM/TI NotifyFramePop() on
>>>>>>>>> the
>>>>>>>>> same target thread, what keeps the threads from messing with
>>>>>>>>> the list of frame pops simultaneously or messing with the
>>>>>>>>> thread enabled events bits in parallel?
>>>>>>>>>
>>>>>>>>> I suspect that this might also be an issue for
>>>>>>>>> JvmtiEventController::clear_frame_pop() and
>>>>>>>>> JvmtiEventController::clear_to_frame_pop() also.
>>>>>>>>>
>>>>>>>>> src/share/vm/prims/jvmtiThreadState.cpp
>>>>>>>>> No comments.
>>>>>>>>>
>>>>>>>>> Dan
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Summary:
>>>>>>>>>>
>>>>>>>>>> It is the 2-nd round of review because the JTREG com/sun/jdi
>>>>>>>>>> tests discovered a regression
>>>>>>>>>> in the first round change. The issue was in the
>>>>>>>>>> JvmtiEventController::clear_frame_pop()
>>>>>>>>>> lock synchronization that is not allowed at safepoints.
>>>>>>>>>>
>>>>>>>>>> As a result I've changed the JvmtiEnv::NotifyFramePop to use a
>>>>>>>>>> VM operation for safety.
>>>>>>>>>> Also, I've removed the lock synchronization from the 3
>>>>>>>>>> impacted
>>>>>>>>>> JvmtiEventController::
>>>>>>>>>> functions: set_frame_pop(), clear_frame_pop() and
>>>>>>>>>> clear_to_frame_pop().
>>>>>>>>>>
>>>>>>>>>> Testing:
>>>>>>>>>> In progress: nsk.jvmti, nsk.jdi, nsk.jdwp, JTreg com/sun/jdi
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Serguei
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2/25/14 12:43 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>> Please, review the fix for:
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-6471769
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Open webrev:
>>>>>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/6471769-JVMTI-DEPTH.1
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Summary:
>>>>>>>>>>>
>>>>>>>>>>> This is another Test Stabilization issue.
>>>>>>>>>>> The fix is very similar to other JVMTI stabilization fixes.
>>>>>>>>>>> It is to use safepoints for updating the PopFrame data
>>>>>>>>>>> instead
>>>>>>>>>>> of relying on the
>>>>>>>>>>> suspend equivalent condition mechanism
>>>>>>>>>>> (JvmtiEnv::is_thread_fully_suspended())
>>>>>>>>>>> which is not adequate from the reliability point of view.
>>>>>>>>>>>
>>>>>>>>>>> Testing:
>>>>>>>>>>> In progress: nsk.jvmti, nsk.jdi, nsk.jdwp, JTreg com/sun/jdi
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Serguei
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>>
>
More information about the serviceability-dev
mailing list