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
Wed May 13 05:42:50 UTC 2020
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 serviceability-dev
mailing list