RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
David Holmes
david.holmes at oracle.com
Thu May 14 05:29:22 UTC 2020
> 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.holmes at oracle.com>
> Sent: Mittwoch, 13. Mai 2020 07:43
> To: Reingruber, Richard <richard.reingruber at sap.com>; serviceability-dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; hotspot-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net; hotspot-gc-dev at 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.holmes at oracle.com>
>> Sent: Montag, 4. Mai 2020 08:51
>> To: Reingruber, Richard <richard.reingruber at sap.com>; Yasumasa Suenaga <suenaga at oss.nttdata.com>; Patricio Chilano <patricio.chilano.mateo at oracle.com>; serguei.spitsyn at oracle.com; Vladimir Ivanov <vladimir.x.ivanov at oracle.com>; serviceability-dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; hotspot-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net; hotspot-gc-dev at 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.holmes at oracle.com>
>>> Sent: Montag, 27. April 2020 07:16
>>> To: Reingruber, Richard <richard.reingruber at sap.com>; Yasumasa Suenaga <suenaga at oss.nttdata.com>; Patricio Chilano <patricio.chilano.mateo at oracle.com>; serguei.spitsyn at oracle.com; Vladimir Ivanov <vladimir.x.ivanov at oracle.com>; serviceability-dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; hotspot-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net; hotspot-gc-dev at 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 <suenaga at oss.nttdata.com>
>>>> Sent: Freitag, 24. April 2020 17:23
>>>> To: Reingruber, Richard <richard.reingruber at sap.com>; Patricio Chilano <patricio.chilano.mateo at oracle.com>; serguei.spitsyn at oracle.com; Vladimir Ivanov <vladimir.x.ivanov at oracle.com>; serviceability-dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; hotspot-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net; hotspot-gc-dev at 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 <suenaga at oss.nttdata.com>
>>>>> Sent: Freitag, 24. April 2020 13:34
>>>>> To: Reingruber, Richard <richard.reingruber at sap.com>; Patricio Chilano <patricio.chilano.mateo at oracle.com>; serguei.spitsyn at oracle.com; Vladimir Ivanov <vladimir.x.ivanov at oracle.com>; serviceability-dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; hotspot-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net; hotspot-gc-dev at 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-bounces at openjdk.java.net> On Behalf Of Reingruber, Richard
>>>>>> Sent: Freitag, 14. Februar 2020 19:47
>>>>>> To: Patricio Chilano <patricio.chilano.mateo at oracle.com>; serviceability-dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; hotspot-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net; hotspot-gc-dev at 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.mateo at oracle.com>
>>>>>> Sent: Freitag, 14. Februar 2020 15:54
>>>>>> To: Reingruber, Richard <richard.reingruber at sap.com>; serviceability-dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; hotspot-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net; hotspot-gc-dev at 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.mateo at oracle.com>
>>>>>>> Sent: Donnerstag, 13. Februar 2020 18:47
>>>>>>> To: Reingruber, Richard <richard.reingruber at sap.com>; serviceability-dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; hotspot-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net; hotspot-gc-dev at 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
>>>>>>
More information about the hotspot-gc-dev
mailing list