Hi Yasumasa, Patricio, > >> 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.
> Thanks for your information. > I tested my patch with both vmTestbase/nsk/jvmti/PopFrame and > vmTestbase/nsk/jvmti/NotifyFramePop. > I will modify and will test it after yours. Thanks :) > > 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. > I think JvmtiEventController::set_frame_pop() should hold > JvmtiThreadState_lock because it affects other JVMTI operation especially > FramePop event. Yes. To me it is unclear what synchronization is necessary, if it is called during a handshake. And also I'm unsure if a thread should do safepoint checks while executing a handshake. @Patricio, coming back to my question [1]: In the example you gave in your answer [2]: the java thread would execute a vm operation during a direct handshake operation, while the VMThread is actually in the middle of a VM_HandshakeAllThreads operation, waiting to handshake the same handshakee: why can't the VMThread just proceed? The handshakee would be safepoint safe, wouldn't it? Thanks, Richard. [1] https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301677&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301677 [2] https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301763&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301763 -----Original Message----- From: Yasumasa Suenaga <suen...@oss.nttdata.com> Sent: Freitag, 24. April 2020 17:23 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, On 2020/04/24 23:44, Reingruber, Richard wrote: > 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. Thanks for your information. I tested my patch with both vmTestbase/nsk/jvmti/PopFrame and vmTestbase/nsk/jvmti/NotifyFramePop. I will modify and will test it after yours. > 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. I think JvmtiEventController::set_frame_pop() should hold JvmtiThreadState_lock because it affects other JVMTI operation especially FramePop event. Thanks, Yasumasa > 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 >>
