Hi Richard,
I had already reviewed webrev.1. Looks good to me.
Thanks for contributing it.
Best regards,
Martin
> -----Original Message-----
> From: hotspot-runtime-dev <hotspot-runtime-dev-
> boun...@openjdk.java.net> On Behalf Of Reingruber, Richard
> Sent: Montag, 4. Mai 2020 12:33
> To: David Holmes <david.hol...@oracle.com>; serviceability-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@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
>
> // Trimmed the list of recipients. If the list gets too long then the message
> needs to be approved
> // by a moderator.
>
> Hi David,
>
> > 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
>
> 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-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@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-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@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.syst
> em.issuetabpanels:comment-tabpanel#comment-14301677
> >>
> >> [2] https://bugs.openjdk.java.net/browse/JDK-
> 8230594?focusedCommentId=14301763&page=com.atlassian.jira.plugin.syst
> em.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-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@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.syst
> em.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-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@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-
> d...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-
> d...@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-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@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-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@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
> >>>>
