On 2/27/14 10:04 PM, David Holmes wrote:
Hi Serguei,
On 28/02/2014 1:50 PM, serguei.spit...@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
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.spit...@oracle.com wrote:
On 2/27/14 1:03 PM, serguei.spit...@oracle.com wrote:
On 2/27/14 12:28 PM, serguei.spit...@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.spit...@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.spit...@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
