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

Reingruber, Richard richard.reingruber at sap.com
Thu May 14 07:48:43 UTC 2020


Hi Serguei,

> 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.

Sure. I'd be happy if you could look at it again.

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

The patch goes through extensive testing here at SAP every night since many weeks. Still it would be
great if you could run full tier5.

I'll wait then for a view more thumbs...

Thanks, Richard.

-----Original Message-----
From: serguei.spitsyn at oracle.com <serguei.spitsyn at oracle.com> 
Sent: Donnerstag, 14. Mai 2020 00:32
To: Reingruber, Richard <richard.reingruber at sap.com>; Patricio Chilano <patricio.chilano.mateo 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,

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