Ok. Thanks for the feedback anyways. Cheers, Richard.
-----Original Message----- From: David Holmes <david.hol...@oracle.com> Sent: Donnerstag, 14. Mai 2020 07:29 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 > Still not a review, or is it now? I'd say still not a review as I'm only looking at the general structure. Cheers, David On 14/05/2020 1:37 am, Reingruber, Richard wrote: > 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 >>>>>>
