3-nd round RFR 6471769: Error: assert(_cur_stack_depth == count_frames(), "cur_stack_depth out of sync")
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Feb 28 13:33:46 PST 2014
On 2/28/14 1:26 PM, Daniel D. Daugherty wrote:
> 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.
Ok, I'll add a comment before the push.
The latest public webrev with the simplification fixes suggested by
David (no above comment yet):
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/6471769-JVMTI-DEPTH.4/
Thanks,
Serguei
>
> 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 hotspot-dev
mailing list