Hi Yasumasa, > I will send review request to replace VM_SetFramePop to handshake in early > next week in JDK-8242427. > Does it help you? I think it gives you to remove workaround.
I think it would not help that much. Note that when replacing VM_SetFramePop with a direct handshake you could not just execute VM_EnterInterpOnlyMode as a nested vm operation [1]. So you would have to change/replace VM_EnterInterpOnlyMode and I would have to adapt to these changes. Also my first impression was that it won't be that easy from a synchronization point of view to replace VM_SetFramePop with a direct handshake. E.g. VM_SetFramePop::doit() indirectly calls JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop fpop) where JvmtiThreadState_lock is acquired with safepoint check, if not at safepoint. It's not directly clear to me, how this has to be handled. So it appears to me that it would be easier to push JDK-8242427 after this (JDK-8238585). > (The patch is available, but I want to see the result of PIT in this weekend > whether JDK-8242425 works fine.) Would be interesting to see how you handled the issues above :) Thanks, Richard. [1] See question in comment https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14302030&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14302030 -----Original Message----- From: Yasumasa Suenaga <suen...@oss.nttdata.com> Sent: Freitag, 24. April 2020 13:34 To: Reingruber, Richard <richard.reingru...@sap.com>; Patricio Chilano <patricio.chilano.ma...@oracle.com>; serguei.spit...@oracle.com; Vladimir Ivanov <vladimir.x.iva...@oracle.com>; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 will send review request to replace VM_SetFramePop to handshake in early next week in JDK-8242427. Does it help you? I think it gives you to remove workaround. (The patch is available, but I want to see the result of PIT in this weekend whether JDK-8242425 works fine.) Thanks, Yasumasa On 2020/04/24 17:18, Reingruber, Richard wrote: > Hi Patricio, Vladimir, and Serguei, > > now that direct handshakes are available, I've updated the patch to make use > of them. > > In addition I have done some clean-up changes I missed in the first webrev. > > Finally I have implemented the workaround suggested by Patricio to avoid > nesting the handshake > into the vm operation VM_SetFramePop [1] > > Kindly review again: > > Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ > Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ > > I updated the JBS item explaining why the vm operation VM_EnterInterpOnlyMode > can be replaced with a > direct handshake: > > JBS: https://bugs.openjdk.java.net/browse/JDK-8238585 > > Testing: > > * JCK and JTREG tests, also in Xcomp mode with fastdebug and release builds > on all platforms. > > * Submit-repo: mach5-one-rrich-JDK-8238585-20200423-1436-10441737 > > Thanks, > Richard. > > [1] An assertion in Handshake::execute_direct() fails, if called be VMThread, > because it is no JavaThread. > > -----Original Message----- > From: hotspot-dev <hotspot-dev-boun...@openjdk.java.net> On Behalf Of > Reingruber, Richard > Sent: Freitag, 14. Februar 2020 19:47 > To: Patricio Chilano <patricio.chilano.ma...@oracle.com>; > serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; > hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; > hotspot-gc-...@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 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.ma...@oracle.com> > Sent: Freitag, 14. Februar 2020 15:54 > To: Reingruber, Richard <richard.reingru...@sap.com>; > serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; > hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; > hotspot-gc-...@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.ma...@oracle.com> >> Sent: Donnerstag, 13. Februar 2020 18:47 >> To: Reingruber, Richard <richard.reingru...@sap.com>; >> serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; >> hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; >> hotspot-gc-...@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 >
