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
