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

[Reingruber, Richard]

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?

  > 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 :)

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