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 hotspot-gc-dev mailing list