RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Yasumasa Suenaga
suenaga at oss.nttdata.com
Fri Apr 24 15:23:06 UTC 2020
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-runtime-dev
mailing list