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

Fri Feb 14 14:53:52 UTC 2020

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