RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Wed May 13 22:31:47 UTC 2020


Hi Richard,

Thank you for the bug report update - it is helpful.
The fix/update looks good in general but I need more time to check some 
points.

I'm thinking it would be more safe to run full tier5.
I can do it after you get all thumbs ups.

Thanks,
Serguei


On 4/24/20 01: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