On 2020/08/29 5:54, Patricio Chilano wrote:
Hi Yasumasa,
On 8/27/20 10:18 PM, Yasumasa Suenaga wrote:
Hi Patricio,
On 2020/08/27 15:20, Patricio Chilano wrote:
Hi Yasumasa,
On 8/26/20 8:57 PM, Yasumasa Suenaga wrote:
Hi Patricio,
Thanks for your review, but webrev.00 has been rotten.
Can you review webrev.02?
webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.02/
diff between webrev.00 and webrev.01:
http://hg.openjdk.java.net/jdk/submit/rev/7facd1dd39d6
diff between webrev.01 and webrev.02:
http://hg.openjdk.java.net/jdk/submit/rev/2ef7feb5681f
Looks good to me. Can JvmtiEventController::set_frame_pop(),
JvmtiEventController::clear_frame_pop() and
JvmtiEventController::clear_to_frame_pop() still be called at a safepoint?
No, and also I checked them with assert(JvmtiThreadState_lock->is_locked()).
webrev is here:
webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.03/
diff from previous webrev:
http://hg.openjdk.java.net/jdk/submit/rev/2a2c02ada080
I see that you will change them with assert_lock_strong() as David suggested
which I think is good.
Thanks Patricio!
I will fix that in next webrev.
Yasumasa
Thanks,
Patricio
Thanks,
Yasumasa
Thanks,
Patricio
Thanks,
Yasumasa
On 2020/08/27 7:50, Patricio Chilano wrote:
Hi Yasumasa,
On 8/26/20 4:34 AM, 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/
The changes look good to me, thanks for fixing them.
Patricio
I understand David said same concerns as Patricio about active handshaker. This
webrev checks active handshaker is current thread or not.
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
