Hi David, > On 4/05/2020 8:33 pm, Reingruber, Richard wrote: > > // Trimmed the list of recipients. If the list gets too long then the > > message needs to be approved > > // by a moderator.
> Yes I noticed that too :) In general if you send to hotspot-dev you > shouldn't need to also send to hotspot-X-dev. Makes sense. Will do so next time. > > > > This would be the post with the current webrev.1 > > > > > > http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html > <sigh> Sorry I missed that update. Okay so this is working with direct > handshakes now. > One style nit in jvmtiThreadState.cpp: > assert(SafepointSynchronize::is_at_safepoint() || > ! (JavaThread *)Thread::current() == get_thread() || > ! Thread::current() == get_thread()->active_handshaker(), > ! "bad synchronization with owner thread"); > the ! lines should ident as follows > assert(SafepointSynchronize::is_at_safepoint() || > (JavaThread *)Thread::current() == get_thread() || > Thread::current() == get_thread()->active_handshaker(), > ! "bad synchronization with owner thread"); Sure. > Lets see how this plays out. Hopefully not too bad... :) >> Not a review but some general commentary ... Still not a review, or is it now? Thanks, Richard. -----Original Message----- From: David Holmes <david.hol...@oracle.com> Sent: Mittwoch, 13. Mai 2020 07:43 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 On 4/05/2020 8:33 pm, Reingruber, Richard wrote: > // Trimmed the list of recipients. If the list gets too long then the message > needs to be approved > // by a moderator. Yes I noticed that too :) In general if you send to hotspot-dev you shouldn't need to also send to hotspot-X-dev. > Hi David, Hi Richard, >> On 28/04/2020 12:09 am, Reingruber, Richard wrote: >>> Hi David, >>> >>>> Not a review but some general commentary ... >>> >>> That's welcome. > >> Having had to take an even closer look now I have a review comment too :) > >> src/hotspot/share/prims/jvmtiThreadState.cpp > >> void JvmtiThreadState::invalidate_cur_stack_depth() { >> ! assert(SafepointSynchronize::is_at_safepoint() || >> ! (Thread::current()->is_VM_thread() && >> get_thread()->is_vmthread_processing_handshake()) || >> (JavaThread *)Thread::current() == get_thread(), >> "must be current thread or at safepoint"); > > You're looking at an outdated webrev, I'm afraid. > > This would be the post with the current webrev.1 > > > http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html <sigh> Sorry I missed that update. Okay so this is working with direct handshakes now. One style nit in jvmtiThreadState.cpp: assert(SafepointSynchronize::is_at_safepoint() || ! (JavaThread *)Thread::current() == get_thread() || ! Thread::current() == get_thread()->active_handshaker(), ! "bad synchronization with owner thread"); the ! lines should ident as follows assert(SafepointSynchronize::is_at_safepoint() || (JavaThread *)Thread::current() == get_thread() || Thread::current() == get_thread()->active_handshaker(), ! "bad synchronization with owner thread"); Lets see how this plays out. Cheers, David > Thanks, Richard. > > -----Original Message----- > From: David Holmes <david.hol...@oracle.com> > Sent: Montag, 4. Mai 2020 08:51 > To: Reingruber, Richard <richard.reingru...@sap.com>; Yasumasa Suenaga > <suen...@oss.nttdata.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 28/04/2020 12:09 am, Reingruber, Richard wrote: >> Hi David, >> >>> Not a review but some general commentary ... >> >> That's welcome. > > Having had to take an even closer look now I have a review comment too :) > > src/hotspot/share/prims/jvmtiThreadState.cpp > > void JvmtiThreadState::invalidate_cur_stack_depth() { > ! assert(SafepointSynchronize::is_at_safepoint() || > ! (Thread::current()->is_VM_thread() && > get_thread()->is_vmthread_processing_handshake()) || > (JavaThread *)Thread::current() == get_thread(), > "must be current thread or at safepoint"); > > The message needs updating to include handshakes. > > More below ... > >>> On 25/04/2020 2:08 am, Reingruber, Richard wrote: >>>> 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. >> >>> I'm growing increasingly concerned that use of direct handshakes to >>> replace VM operations needs a much greater examination for correctness >>> than might initially be thought. I see a number of issues: >> >> I agree. I'll address your concerns in the context of this review thread for >> JDK-8238585 below. >> >> In addition I would suggest to take the general part of the discussion to a >> dedicated thread or to >> the review thread for JDK-8242427. I would like to keep this thread closer >> to its subject. > > I will focus on the issues in the context of this particular change > then, though the issues themselves are applicable to all handshake > situations (and more so with direct handshakes). This is mostly just > discussion. > >>> First, the VMThread executes (most) VM operations with a clean stack in >>> a clean state, so it has lots of room to work. If we now execute the >>> same logic in a JavaThread then we risk hitting stackoverflows if >>> nothing else. But we are also now executing code in a JavaThread and so >>> we have to be sure that code is not going to act differently (in a bad >>> way) if executed by a JavaThread rather than the VMThread. For example, >>> may it be possible that if executing in the VMThread we defer some >>> activity that might require execution of Java code, or else hand it off >>> to one of the service threads? If we execute that code directly in the >>> current JavaThread instead we may not be in a valid state (e.g. consider >>> re-entrancy to various subsystems that is not allowed). >> >> It is not too complex, what EnterInterpOnlyModeClosure::do_thread() is >> doing. I already added a >> paragraph to the JBS-Item [1] explaining why the direct handshake is >> sufficient from a >> synchronization point of view. > > Just to be clear, your proposed change is not using a direct handshake. > >> Furthermore the stack is walked and the return pc of compiled frames is >> replaced with the address of >> the deopt handler. >> >> I can't see why this cannot be done with a direct handshake. Something very >> similar is already done >> in JavaThread::deoptimize_marked_methods() which is executed as part of an >> ordinary handshake. > > Note that existing non-direct handshakes may also have issues that not > have been fully investigated. > >> The demand on stack-space should be very modest. I would not expect a higher >> risk for stackoverflow. > > For the target thread if you use more stack than would be used stopping > at a safepoint then you are at risk. For the thread initiating the > direct handshake if you use more stack than would be used enqueuing a VM > operation, then you are at risk. As we have not quantified these > numbers, nor have any easy way to establish the stack use of the actual > code to be executed, we're really just hoping for the best. This is a > general problem with handshakes that needs to be investigated more > deeply. As a simple, general, example just imagine if the code involves > logging that might utilise an on-stack buffer. > >>> Second, we have this question mark over what happens if the operation >>> hits further safepoint or handshake polls/checks? Are there constraints >>> on what is allowed here? How can we recognise this problem may exist and >>> so deal with it? >> >> The thread in EnterInterpOnlyModeClosure::do_thread() can't become >> safepoint/handshake safe. I >> tested locally test/hotspot/jtreg:vmTestbase_nsk_jvmti with a >> NoSafepointVerifier. > > That's good to hear but such tests are not exhaustive, they will detect > if you do reach a safepoint/handshake but they can't prove that you > cannot reach one. What you have done is necessary but may not be > sufficient. Plus you didn't actually add the NSV to the code - is there > a reason we can't actually keep it in do_thread? (I'm not sure if the > NSV also acts as a NoHandshakeVerifier?) > >>> Third, while we are generally considering what appear to be >>> single-thread operations, which should be amenable to a direct >>> handshake, we also have to be careful that some of the code involved >>> doesn't already expect/assume we are at a safepoint - e.g. a VM op may >>> not need to take a lock where a direct handshake might! >> >> See again my arguments in the JBS item [1]. > > Yes I see the reasoning and that is good. My point is a general one as > it may not be obvious when such assumptions exist in the current code. > > Thanks, > David > >> Thanks, >> Richard. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8238585 >> >> -----Original Message----- >> From: David Holmes <david.hol...@oracle.com> >> Sent: Montag, 27. April 2020 07:16 >> To: Reingruber, Richard <richard.reingru...@sap.com>; Yasumasa Suenaga >> <suen...@oss.nttdata.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 all, >> >> Not a review but some general commentary ... >> >> On 25/04/2020 2:08 am, Reingruber, Richard wrote: >>> 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. >> >> I'm growing increasingly concerned that use of direct handshakes to >> replace VM operations needs a much greater examination for correctness >> than might initially be thought. I see a number of issues: >> >> First, the VMThread executes (most) VM operations with a clean stack in >> a clean state, so it has lots of room to work. If we now execute the >> same logic in a JavaThread then we risk hitting stackoverflows if >> nothing else. But we are also now executing code in a JavaThread and so >> we have to be sure that code is not going to act differently (in a bad >> way) if executed by a JavaThread rather than the VMThread. For example, >> may it be possible that if executing in the VMThread we defer some >> activity that might require execution of Java code, or else hand it off >> to one of the service threads? If we execute that code directly in the >> current JavaThread instead we may not be in a valid state (e.g. consider >> re-entrancy to various subsystems that is not allowed). >> >> Second, we have this question mark over what happens if the operation >> hits further safepoint or handshake polls/checks? Are there constraints >> on what is allowed here? How can we recognise this problem may exist and >> so deal with it? >> >> Third, while we are generally considering what appear to be >> single-thread operations, which should be amenable to a direct >> handshake, we also have to be careful that some of the code involved >> doesn't already expect/assume we are at a safepoint - e.g. a VM op may >> not need to take a lock where a direct handshake might! >> >> Cheers, >> David >> ----- >> >>> @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 >>>>>