[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don'
From: "Reingruber, Richard" <richard.reingruber () sap ! com>
Date: 2020-02-14 18:47:20
Message-ID: AM0PR0202MB33318BC60E3460662D42A40D9B150 () AM0PR0202MB3331 ! eurprd02 ! prod ! outlook ! com
[Download RAW message or body]
Hi Patricio,
> > I'm really glad you noticed the problematic nesting. This seems to be a general \
issue: currently a > > handshake cannot be nested in a vm operation. Maybe it should \
be asserted in the > > Handshake::execute() methods that they are not called by the \
vm thread evaluating a vm operation? > >
> > > Alternatively I think you could do something similar to what we do in
> > > Deoptimization::deoptimize_all_marked():
> > >
> > > EnterInterpOnlyModeClosure hs;
> > > if (SafepointSynchronize::is_at_safepoint()) {
> > > hs.do_thread(state->get_thread());
> > > } else {
> > > Handshake::execute(&hs, state->get_thread());
> > > }
> > > (you could pass “EnterInterpOnlyModeClosure” directly to the
> > > HandshakeClosure() constructor)
> >
> > Maybe this could be used also in the Handshake::execute() methods as general \
solution? > Right, we could also do that. Avoiding to clear the polling page in
> HandshakeState::clear_handshake() should be enough to fix this issue and
> execute a handshake inside a safepoint, but adding that "if" statement
> in Hanshake::execute() sounds good to avoid all the extra code that we
> go through when executing a handshake. I filed 8239084 to make that change.
Thanks for taking care of this and creating the RFE.
>
> > > I don’t know JVMTI code so I’m not sure if VM_EnterInterpOnlyMode is
> > > always called in a nested operation or just sometimes.
> >
> > At least one execution path without vm operation exists:
> >
> > JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState *) : \
void > > JvmtiEventControllerPrivate::recompute_thread_enabled(JvmtiThreadState \
*) : jlong > > JvmtiEventControllerPrivate::recompute_enabled() : void
> > JvmtiEventControllerPrivate::change_field_watch(jvmtiEvent, bool) : \
void (2 matches) > > JvmtiEventController::change_field_watch(jvmtiEvent, \
bool) : void > > JvmtiEnv::SetFieldAccessWatch(fieldDescriptor *) : \
jvmtiError > > jvmti_SetFieldAccessWatch(jvmtiEnv *, jclass, \
jfieldID) : jvmtiError > >
> > I tend to revert back to VM_EnterInterpOnlyMode as it wasn't my main intent to \
replace it with a > > handshake, but to avoid making the compiled methods on stack \
not_entrant.... unless I'm further > > encouraged to do it with a handshake :)
> Ah! I think you can still do it with a handshake with the
> Deoptimization::deoptimize_all_marked() like solution. I can change the
> if-else statement with just the Handshake::execute() call in 8239084.
> But up to you. : )
Well, I think that's enough encouragement :)
I'll wait for 8239084 and try then again.
(no urgency and all)
Thanks,
Richard.
-----Original Message-----
From: Patricio Chilano <patricio.chilano.mateo@oracle.com>
Sent: Freitag, 14. Februar 2020 15:54
To: Reingruber, Richard <richard.reingruber@sap.com>; \
serviceability-dev@openjdk.java.net; hotspot-compiler-dev@openjdk.java.net; \
hotspot-dev@openjdk.java.net; hotspot-runtime-dev@openjdk.java.net; \
hotspot-gc-dev@openjdk.java.net
Subject: Re: RFR(S) 8238585: Use handshake for \
JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods \
on stack not_entrant
Hi Richard,
On 2/14/20 9:58 AM, Reingruber, Richard wrote:
> Hi Patricio,
>
> thanks for having a look.
>
> > I’m only commenting on the handshake changes.
> > I see that operation VM_EnterInterpOnlyMode can be called inside
> > operation VM_SetFramePop which also allows nested operations. Here is a
> > comment in VM_SetFramePop definition:
> >
> > // Nested operation must be allowed for the VM_EnterInterpOnlyMode that is
> > // called from the JvmtiEventControllerPrivate::recompute_thread_enabled.
> >
> > So if we change VM_EnterInterpOnlyMode to be a handshake, then now we
> > could have a handshake inside a safepoint operation. The issue I see
> > there is that at the end of the handshake the polling page of the target
> > thread could be disarmed. So if the target thread happens to be in a
> > blocked state just transiently and wakes up then it will not stop for
> > the ongoing safepoint. Maybe I can file an RFE to assert that the
> > polling page is armed at the beginning of disarm_safepoint().
>
> I'm really glad you noticed the problematic nesting. This seems to be a general \
> issue: currently a handshake cannot be nested in a vm operation. Maybe it should be \
> asserted in the Handshake::execute() methods that they are not called by the vm \
> thread evaluating a vm operation?
> > Alternatively I think you could do something similar to what we do in
> > Deoptimization::deoptimize_all_marked():
> >
> > EnterInterpOnlyModeClosure hs;
> > if (SafepointSynchronize::is_at_safepoint()) {
> > hs.do_thread(state->get_thread());
> > } else {
> > Handshake::execute(&hs, state->get_thread());
> > }
> > (you could pass “EnterInterpOnlyModeClosure” directly to the
> > HandshakeClosure() constructor)
>
> Maybe this could be used also in the Handshake::execute() methods as general \
> solution?
Right, we could also do that. Avoiding to clear the polling page in
HandshakeState::clear_handshake() should be enough to fix this issue and
execute a handshake inside a safepoint, but adding that "if" statement
in Hanshake::execute() sounds good to avoid all the extra code that we
go through when executing a handshake. I filed 8239084 to make that change.
> > I don’t know JVMTI code so I’m not sure if VM_EnterInterpOnlyMode is
> > always called in a nested operation or just sometimes.
>
> At least one execution path without vm operation exists:
>
> JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState *) : void
> JvmtiEventControllerPrivate::recompute_thread_enabled(JvmtiThreadState *) : jlong
> JvmtiEventControllerPrivate::recompute_enabled() : void
> JvmtiEventControllerPrivate::change_field_watch(jvmtiEvent, bool) : void (2 \
> matches) JvmtiEventController::change_field_watch(jvmtiEvent, bool) : void
> JvmtiEnv::SetFieldAccessWatch(fieldDescriptor *) : jvmtiError
> jvmti_SetFieldAccessWatch(jvmtiEnv *, jclass, jfieldID) : jvmtiError
>
> I tend to revert back to VM_EnterInterpOnlyMode as it wasn't my main intent to \
> replace it with a handshake, but to avoid making the compiled methods on stack \
> not_entrant.... unless I'm further encouraged to do it with a handshake :)
Ah! I think you can still do it with a handshake with the
Deoptimization::deoptimize_all_marked() like solution. I can change the
if-else statement with just the Handshake::execute() call in 8239084.
But up to you. : )
Thanks,
Patricio
> Thanks again,
> Richard.
>
> -----Original Message-----
> From: Patricio Chilano <patricio.chilano.mateo@oracle.com>
> Sent: Donnerstag, 13. Februar 2020 18:47
> To: Reingruber, Richard <richard.reingruber@sap.com>; \
> serviceability-dev@openjdk.java.net; hotspot-compiler-dev@openjdk.java.net; \
> hotspot-dev@openjdk.java.net; hotspot-runtime-dev@openjdk.java.net; \
> hotspot-gc-dev@openjdk.java.net
> Subject: Re: RFR(S) 8238585: Use handshake for \
> JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled \
> methods on stack not_entrant
> Hi Richard,
>
> I’m only commenting on the handshake changes.
> I see that operation VM_EnterInterpOnlyMode can be called inside
> operation VM_SetFramePop which also allows nested operations. Here is a
> comment in VM_SetFramePop definition:
>
> // Nested operation must be allowed for the VM_EnterInterpOnlyMode that is
> // called from the JvmtiEventControllerPrivate::recompute_thread_enabled.
>
> So if we change VM_EnterInterpOnlyMode to be a handshake, then now we
> could have a handshake inside a safepoint operation. The issue I see
> there is that at the end of the handshake the polling page of the target
> thread could be disarmed. So if the target thread happens to be in a
> blocked state just transiently and wakes up then it will not stop for
> the ongoing safepoint. Maybe I can file an RFE to assert that the
> polling page is armed at the beginning of disarm_safepoint().
>
> I think one option could be to remove
> SafepointMechanism::disarm_if_needed() in
> HandshakeState::clear_handshake() and let each JavaThread disarm itself
> for the handshake case.
>
> Alternatively I think you could do something similar to what we do in
> Deoptimization::deoptimize_all_marked():
>
> EnterInterpOnlyModeClosure hs;
> if (SafepointSynchronize::is_at_safepoint()) {
> hs.do_thread(state->get_thread());
> } else {
> Handshake::execute(&hs, state->get_thread());
> }
> (you could pass “EnterInterpOnlyModeClosure” directly to the
> HandshakeClosure() constructor)
>
> I don’t know JVMTI code so I’m not sure if VM_EnterInterpOnlyMode is
> always called in a nested operation or just sometimes.
>
> Thanks,
> Patricio
>
> On 2/12/20 7:23 AM, Reingruber, Richard wrote:
> > // Repost including hotspot runtime and gc lists.
> > // Dean Long suggested to do so, because the enhancement replaces a vm operation
> > // with a handshake.
> > // Original thread: \
> > http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-February/030359.html
> >
> > Hi,
> >
> > could I please get reviews for this small enhancement in hotspot's jvmti \
> > implementation:
> > Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8238585
> >
> > The change avoids making all compiled methods on stack not_entrant when switching \
> > a java thread to interpreter only execution for jvmti purposes. It is sufficient \
> > to deoptimize the compiled frames on stack.
> > Additionally a handshake is used instead of a vm operation to walk the stack and \
> > do the deoptimizations.
> > Testing: JCK and JTREG tests, also in Xcomp mode with fastdebug and release \
> > builds on all platforms.
> > Thanks, Richard.
> >
> > See also my question if anyone knows a reason for making the compiled methods \
> > not_entrant: http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030339.html
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic