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:12:57 PST 2014
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)
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?
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 hotspot-dev
mailing list