RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
I see. Thanks for the explanation :) Richard. -Original Message- From: serguei.spit...@oracle.com Sent: Freitag, 5. Juni 2020 09:31 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 6/5/20 00:18, Reingruber, Richard wrote: > Hi, > >> The mach5 test run is good. > Thanks Serguei and thanks to everybody providing feedback! I just pushed the > change. Great, thanks! > Just curious: is mach5 an alias for tier5? The mach5 is a build and test system which also provides CI. Tier5 is one of the testing levels. > And is this mach5 the same as in "Job: > mach5-one-rrich-JDK-8238585-2-20200604-1334-11519059" which is the > (successful) submit repo job? Yes. I guess all mach5 jobs have this prefix. Thanks, Serguei > > Thanks, > Richard. > > -Original Message- > From: serguei.spit...@oracle.com > Sent: Donnerstag, 4. Juni 2020 04:07 > To: Reingruber, Richard ; > serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; > hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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, > > The mach5 test run is good. > > Thanks, > Serguei > > > On 6/2/20 10:57, Reingruber, Richard wrote: >> Hi Serguei, >> >>> This looks good to me. >> Thanks! >> >> From an earlier mail: >> >>> I'm thinking it would be more safe to run full tier5. >> I guess we're done with reviewing. Would be good if you could run full tier5 >> now. After that I would >> like to push. >> >> Thanks, Richard. >> >> -Original Message- >> From: serguei.spit...@oracle.com >> Sent: Dienstag, 2. Juni 2020 18:55 >> To: Vladimir Kozlov ; Reingruber, Richard >> ; serviceability-dev@openjdk.java.net; >> hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; >> hotspot-gc-...@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, >> >> This looks good to me. >> >> Thanks, >> Serguei >> >> >> On 5/28/20 09:02, Vladimir Kozlov wrote: >>> Vladimir Ivanov is on break currently. >>> It looks good to me. >>> >>> Thanks, >>> Vladimir K >>> >>> On 5/26/20 7:31 AM, Reingruber, Richard wrote: Hi Vladimir, >> Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ > Not an expert in JVMTI code base, so can't comment on the actual > changes. > From JIT-compilers perspective it looks good. I put out webrev.1 a while ago [1]: Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ You originally suggested to use a handshake to switch a thread into interpreter mode [2]. I'm using a direct handshake now, because I think it is the best fit. May I ask if webrev.1 still looks good to you from JIT-compilers perspective? Can I list you as (partial) Reviewer? Thanks, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html [2] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030340.html -Original Message- From: Vladimir Ivanov Sent: Freitag, 7. Februar 2020 09:19 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@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 > Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. Best regards, Vladimir Ivanov > 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
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 6/5/20 00:18, Reingruber, Richard wrote: Hi, The mach5 test run is good. Thanks Serguei and thanks to everybody providing feedback! I just pushed the change. Great, thanks! Just curious: is mach5 an alias for tier5? The mach5 is a build and test system which also provides CI. Tier5 is one of the testing levels. And is this mach5 the same as in "Job: mach5-one-rrich-JDK-8238585-2-20200604-1334-11519059" which is the (successful) submit repo job? Yes. I guess all mach5 jobs have this prefix. Thanks, Serguei Thanks, Richard. -Original Message- From: serguei.spit...@oracle.com Sent: Donnerstag, 4. Juni 2020 04:07 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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, The mach5 test run is good. Thanks, Serguei On 6/2/20 10:57, Reingruber, Richard wrote: Hi Serguei, This looks good to me. Thanks! From an earlier mail: I'm thinking it would be more safe to run full tier5. I guess we're done with reviewing. Would be good if you could run full tier5 now. After that I would like to push. Thanks, Richard. -Original Message- From: serguei.spit...@oracle.com Sent: Dienstag, 2. Juni 2020 18:55 To: Vladimir Kozlov ; Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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, This looks good to me. Thanks, Serguei On 5/28/20 09:02, Vladimir Kozlov wrote: Vladimir Ivanov is on break currently. It looks good to me. Thanks, Vladimir K On 5/26/20 7:31 AM, Reingruber, Richard wrote: Hi Vladimir, Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. I put out webrev.1 a while ago [1]: Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ You originally suggested to use a handshake to switch a thread into interpreter mode [2]. I'm using a direct handshake now, because I think it is the best fit. May I ask if webrev.1 still looks good to you from JIT-compilers perspective? Can I list you as (partial) Reviewer? Thanks, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html [2] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030340.html -Original Message- From: Vladimir Ivanov Sent: Freitag, 7. Februar 2020 09:19 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@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 Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. Best regards, Vladimir Ivanov 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
Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi Richard, The mach5 test run is good. Thanks, Serguei On 6/2/20 10:57, Reingruber, Richard wrote: Hi Serguei, This looks good to me. Thanks! From an earlier mail: I'm thinking it would be more safe to run full tier5. I guess we're done with reviewing. Would be good if you could run full tier5 now. After that I would like to push. Thanks, Richard. -Original Message- From: serguei.spit...@oracle.com Sent: Dienstag, 2. Juni 2020 18:55 To: Vladimir Kozlov ; Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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, This looks good to me. Thanks, Serguei On 5/28/20 09:02, Vladimir Kozlov wrote: Vladimir Ivanov is on break currently. It looks good to me. Thanks, Vladimir K On 5/26/20 7:31 AM, Reingruber, Richard wrote: Hi Vladimir, Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. I put out webrev.1 a while ago [1]: Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ You originally suggested to use a handshake to switch a thread into interpreter mode [2]. I'm using a direct handshake now, because I think it is the best fit. May I ask if webrev.1 still looks good to you from JIT-compilers perspective? Can I list you as (partial) Reviewer? Thanks, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html [2] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030340.html -Original Message- From: Vladimir Ivanov Sent: Freitag, 7. Februar 2020 09:19 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@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 Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. Best regards, Vladimir Ivanov 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
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Excellent. Thanks! Richard. -Original Message- From: serguei.spit...@oracle.com Sent: Dienstag, 2. Juni 2020 20:02 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 6/2/20 10:57, Reingruber, Richard wrote: > Hi Serguei, > >> This looks good to me. > Thanks! > > From an earlier mail: > >> I'm thinking it would be more safe to run full tier5. > I guess we're done with reviewing. Would be good if you could run full tier5 > now. After that I would > like to push. Okay, I'll submit a mach5 job with your fix and let you know about the results. Thanks, Serguei > Thanks, Richard. > > -Original Message- > From: serguei.spit...@oracle.com > Sent: Dienstag, 2. Juni 2020 18:55 > To: Vladimir Kozlov ; Reingruber, Richard > ; serviceability-dev@openjdk.java.net; > hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; > hotspot-gc-...@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, > > This looks good to me. > > Thanks, > Serguei > > > On 5/28/20 09:02, Vladimir Kozlov wrote: >> Vladimir Ivanov is on break currently. >> It looks good to me. >> >> Thanks, >> Vladimir K >> >> On 5/26/20 7:31 AM, Reingruber, Richard wrote: >>> Hi Vladimir, >>> > Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. >>> I put out webrev.1 a while ago [1]: >>> >>> Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ >>> Webrev(delta): >>> http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ >>> >>> You originally suggested to use a handshake to switch a thread into >>> interpreter mode [2]. I'm using >>> a direct handshake now, because I think it is the best fit. >>> >>> May I ask if webrev.1 still looks good to you from JIT-compilers >>> perspective? >>> >>> Can I list you as (partial) Reviewer? >>> >>> Thanks, Richard. >>> >>> [1] >>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html >>> [2] >>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030340.html >>> >>> -Original Message- >>> From: Vladimir Ivanov >>> Sent: Freitag, 7. Februar 2020 09:19 >>> To: Reingruber, Richard ; >>> serviceability-dev@openjdk.java.net; >>> hotspot-compiler-...@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 >>> >>> Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ >>> Not an expert in JVMTI code base, so can't comment on the actual >>> changes. >>> >>> From JIT-compilers perspective it looks good. >>> >>> Best regards, >>> Vladimir Ivanov >>> 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
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 6/2/20 10:57, Reingruber, Richard wrote: Hi Serguei, This looks good to me. Thanks! From an earlier mail: I'm thinking it would be more safe to run full tier5. I guess we're done with reviewing. Would be good if you could run full tier5 now. After that I would like to push. Okay, I'll submit a mach5 job with your fix and let you know about the results. Thanks, Serguei Thanks, Richard. -Original Message- From: serguei.spit...@oracle.com Sent: Dienstag, 2. Juni 2020 18:55 To: Vladimir Kozlov ; Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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, This looks good to me. Thanks, Serguei On 5/28/20 09:02, Vladimir Kozlov wrote: Vladimir Ivanov is on break currently. It looks good to me. Thanks, Vladimir K On 5/26/20 7:31 AM, Reingruber, Richard wrote: Hi Vladimir, Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. I put out webrev.1 a while ago [1]: Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ You originally suggested to use a handshake to switch a thread into interpreter mode [2]. I'm using a direct handshake now, because I think it is the best fit. May I ask if webrev.1 still looks good to you from JIT-compilers perspective? Can I list you as (partial) Reviewer? Thanks, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html [2] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030340.html -Original Message- From: Vladimir Ivanov Sent: Freitag, 7. Februar 2020 09:19 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@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 Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. Best regards, Vladimir Ivanov 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
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi Serguei, > This looks good to me. Thanks! From an earlier mail: > I'm thinking it would be more safe to run full tier5. I guess we're done with reviewing. Would be good if you could run full tier5 now. After that I would like to push. Thanks, Richard. -Original Message- From: serguei.spit...@oracle.com Sent: Dienstag, 2. Juni 2020 18:55 To: Vladimir Kozlov ; Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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, This looks good to me. Thanks, Serguei On 5/28/20 09:02, Vladimir Kozlov wrote: > Vladimir Ivanov is on break currently. > It looks good to me. > > Thanks, > Vladimir K > > On 5/26/20 7:31 AM, Reingruber, Richard wrote: >> Hi Vladimir, >> Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ >> >>> Not an expert in JVMTI code base, so can't comment on the actual >>> changes. >> >>> From JIT-compilers perspective it looks good. >> >> I put out webrev.1 a while ago [1]: >> >> Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ >> Webrev(delta): >> http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ >> >> You originally suggested to use a handshake to switch a thread into >> interpreter mode [2]. I'm using >> a direct handshake now, because I think it is the best fit. >> >> May I ask if webrev.1 still looks good to you from JIT-compilers >> perspective? >> >> Can I list you as (partial) Reviewer? >> >> Thanks, Richard. >> >> [1] >> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html >> [2] >> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030340.html >> >> -Original Message- >> From: Vladimir Ivanov >> Sent: Freitag, 7. Februar 2020 09:19 >> To: Reingruber, Richard ; >> serviceability-dev@openjdk.java.net; >> hotspot-compiler-...@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 >> >> >>> Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ >> >> Not an expert in JVMTI code base, so can't comment on the actual >> changes. >> >> From JIT-compilers perspective it looks good. >> >> Best regards, >> Vladimir Ivanov >> >>> 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 >>> >>> >>>
Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi Richard, This looks good to me. Thanks, Serguei On 5/28/20 09:02, Vladimir Kozlov wrote: Vladimir Ivanov is on break currently. It looks good to me. Thanks, Vladimir K On 5/26/20 7:31 AM, Reingruber, Richard wrote: Hi Vladimir, Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. I put out webrev.1 a while ago [1]: Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ You originally suggested to use a handshake to switch a thread into interpreter mode [2]. I'm using a direct handshake now, because I think it is the best fit. May I ask if webrev.1 still looks good to you from JIT-compilers perspective? Can I list you as (partial) Reviewer? Thanks, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html [2] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030340.html -Original Message- From: Vladimir Ivanov Sent: Freitag, 7. Februar 2020 09:19 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@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 Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. Best regards, Vladimir Ivanov 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
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Thanks for the info, Vladimir, and for looking at the webrev. Best regards, Richard. -Original Message- From: Vladimir Kozlov Sent: Donnerstag, 28. Mai 2020 18:03 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 Vladimir Ivanov is on break currently. It looks good to me. Thanks, Vladimir K On 5/26/20 7:31 AM, Reingruber, Richard wrote: > Hi Vladimir, > >>> Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ > >> Not an expert in JVMTI code base, so can't comment on the actual changes. > >> From JIT-compilers perspective it looks good. > > I put out webrev.1 a while ago [1]: > > Webrev:http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ > Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ > > You originally suggested to use a handshake to switch a thread into > interpreter mode [2]. I'm using > a direct handshake now, because I think it is the best fit. > > May I ask if webrev.1 still looks good to you from JIT-compilers perspective? > > Can I list you as (partial) Reviewer? > > Thanks, Richard. > > [1] > http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html > [2] > http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030340.html > > -Original Message- > From: Vladimir Ivanov > Sent: Freitag, 7. Februar 2020 09:19 > To: Reingruber, Richard ; > serviceability-dev@openjdk.java.net; hotspot-compiler-...@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 > > >> Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ > > Not an expert in JVMTI code base, so can't comment on the actual changes. > > From JIT-compilers perspective it looks good. > > Best regards, > Vladimir Ivanov > >> 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 >>
Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Vladimir Ivanov is on break currently. It looks good to me. Thanks, Vladimir K On 5/26/20 7:31 AM, Reingruber, Richard wrote: Hi Vladimir, Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. I put out webrev.1 a while ago [1]: Webrev:http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ You originally suggested to use a handshake to switch a thread into interpreter mode [2]. I'm using a direct handshake now, because I think it is the best fit. May I ask if webrev.1 still looks good to you from JIT-compilers perspective? Can I list you as (partial) Reviewer? Thanks, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html [2] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030340.html -Original Message- From: Vladimir Ivanov Sent: Freitag, 7. Februar 2020 09:19 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@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 Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. Best regards, Vladimir Ivanov 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
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi Vladimir, > > Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ > Not an expert in JVMTI code base, so can't comment on the actual changes. > From JIT-compilers perspective it looks good. I put out webrev.1 a while ago [1]: Webrev:http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ You originally suggested to use a handshake to switch a thread into interpreter mode [2]. I'm using a direct handshake now, because I think it is the best fit. May I ask if webrev.1 still looks good to you from JIT-compilers perspective? Can I list you as (partial) Reviewer? Thanks, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html [2] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030340.html -Original Message- From: Vladimir Ivanov Sent: Freitag, 7. Februar 2020 09:19 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@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 > Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. Best regards, Vladimir Ivanov > 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 >
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
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.spit...@oracle.com
Sent: Donnerstag, 14. Mai 2020 00:32
To: Reingruber, Richard ; Patricio Chilano
; Vladimir Ivanov
; serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 On Behalf Of
> Reingruber, Richard
> Sent: Freitag, 14. Februar 2020 19:47
> To: Patricio Chilano ;
> serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
> hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
> hotspot-gc-...@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(, 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)
>> >
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Ok. Thanks for the feedback anyways.
Cheers, Richard.
-Original Message-
From: David Holmes
Sent: Donnerstag, 14. Mai 2020 07:29
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@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
> Still not a review, or is it now?
I'd say still not a review as I'm only looking at the general structure.
Cheers,
David
On 14/05/2020 1:37 am, Reingruber, Richard wrote:
> Hi David,
>
>> On 4/05/2020 8:33 pm, Reingruber, Richard wrote:
>>> // Trimmed the list of recipients. If the list gets too long then the
>>> message needs to be approved
>>> // by a moderator.
>
>> Yes I noticed that too :) In general if you send to hotspot-dev you
>> shouldn't need to also send to hotspot-X-dev.
>
> Makes sense. Will do so next time.
>
>>>
>>> This would be the post with the current webrev.1
>>>
>>>
>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html
>
>> Sorry I missed that update. Okay so this is working with direct
>> handshakes now.
>
>> One style nit in jvmtiThreadState.cpp:
>
>> assert(SafepointSynchronize::is_at_safepoint() ||
>> ! (JavaThread *)Thread::current() == get_thread() ||
>> ! Thread::current() == get_thread()->active_handshaker(),
>> ! "bad synchronization with owner thread");
>
>> the ! lines should ident as follows
>
>> assert(SafepointSynchronize::is_at_safepoint() ||
>> (JavaThread *)Thread::current() == get_thread() ||
>> Thread::current() == get_thread()->active_handshaker(),
>>! "bad synchronization with owner thread");
>
> Sure.
>
>> Lets see how this plays out.
>
> Hopefully not too bad... :)
>
>>> Not a review but some general commentary ...
>
> Still not a review, or is it now?
>
> Thanks, Richard.
>
> -Original Message-
> From: David Holmes
> Sent: Mittwoch, 13. Mai 2020 07:43
> To: Reingruber, Richard ;
> serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
> hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
> hotspot-gc-...@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
>
> On 4/05/2020 8:33 pm, Reingruber, Richard wrote:
>> // Trimmed the list of recipients. If the list gets too long then the
>> message needs to be approved
>> // by a moderator.
>
> Yes I noticed that too :) In general if you send to hotspot-dev you
> shouldn't need to also send to hotspot-X-dev.
>
>> Hi David,
>
> Hi Richard,
>
>>> On 28/04/2020 12:09 am, Reingruber, Richard wrote:
Hi David,
> Not a review but some general commentary ...
That's welcome.
>>
>>> Having had to take an even closer look now I have a review comment too :)
>>
>>> src/hotspot/share/prims/jvmtiThreadState.cpp
>>
>>> void JvmtiThreadState::invalidate_cur_stack_depth() {
>>> ! assert(SafepointSynchronize::is_at_safepoint() ||
>>> ! (Thread::current()->is_VM_thread() &&
>>> get_thread()->is_vmthread_processing_handshake()) ||
>>> (JavaThread *)Thread::current() == get_thread(),
>>> "must be current thread or at safepoint");
>>
>> You're looking at an outdated webrev, I'm afraid.
>>
>> This would be the post with the current webrev.1
>>
>>
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html
>
> Sorry I missed that update. Okay so this is working with direct
> handshakes now.
>
> One style nit in jvmtiThreadState.cpp:
>
> assert(SafepointSynchronize::is_at_safepoint() ||
> ! (JavaThread *)Thread::current() == get_thread() ||
> ! Thread::current() == get_thread()->active_handshaker(),
> ! "bad synchronization with owner thread");
>
> the ! lines should ident as follows
>
> assert(SafepointSynchronize::is_at_safepoint() ||
> (JavaThread *)Thread::current() == get_thread() ||
> Thread::current() == get_thread()->active_handshaker(),
>! "bad synchronization with owner thread");
>
> Lets see how this plays out.
>
> Cheers,
> David
>
>> Thanks, Richard.
>>
>> -Original Message-
>> From: David Holmes
>> Sent: Montag, 4. Mai 2020 08:51
>> To: Reingruber, Richard ; Yasumasa Suenaga
>> ; Patricio Chilano
>> ; serguei.spit...@oracle.com; Vladimir
>> Ivanov ; serviceability-dev@openjdk.java.net;
>> hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net;
>> hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net
>> Subject: Re: RFR(S) 8238585: Use handshake for
>> JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make
>> compiled
Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
> Still not a review, or is it now?
I'd say still not a review as I'm only looking at the general structure.
Cheers,
David
On 14/05/2020 1:37 am, Reingruber, Richard wrote:
Hi David,
On 4/05/2020 8:33 pm, Reingruber, Richard wrote:
// Trimmed the list of recipients. If the list gets too long then the message
needs to be approved
// by a moderator.
Yes I noticed that too :) In general if you send to hotspot-dev you
shouldn't need to also send to hotspot-X-dev.
Makes sense. Will do so next time.
This would be the post with the current webrev.1
http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html
Sorry I missed that update. Okay so this is working with direct
handshakes now.
One style nit in jvmtiThreadState.cpp:
assert(SafepointSynchronize::is_at_safepoint() ||
! (JavaThread *)Thread::current() == get_thread() ||
! Thread::current() == get_thread()->active_handshaker(),
! "bad synchronization with owner thread");
the ! lines should ident as follows
assert(SafepointSynchronize::is_at_safepoint() ||
(JavaThread *)Thread::current() == get_thread() ||
Thread::current() == get_thread()->active_handshaker(),
! "bad synchronization with owner thread");
Sure.
Lets see how this plays out.
Hopefully not too bad... :)
Not a review but some general commentary ...
Still not a review, or is it now?
Thanks, Richard.
-Original Message-
From: David Holmes
Sent: Mittwoch, 13. Mai 2020 07:43
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@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
On 4/05/2020 8:33 pm, Reingruber, Richard wrote:
// Trimmed the list of recipients. If the list gets too long then the message
needs to be approved
// by a moderator.
Yes I noticed that too :) In general if you send to hotspot-dev you
shouldn't need to also send to hotspot-X-dev.
Hi David,
Hi Richard,
On 28/04/2020 12:09 am, Reingruber, Richard wrote:
Hi David,
Not a review but some general commentary ...
That's welcome.
Having had to take an even closer look now I have a review comment too :)
src/hotspot/share/prims/jvmtiThreadState.cpp
void JvmtiThreadState::invalidate_cur_stack_depth() {
! assert(SafepointSynchronize::is_at_safepoint() ||
! (Thread::current()->is_VM_thread() &&
get_thread()->is_vmthread_processing_handshake()) ||
(JavaThread *)Thread::current() == get_thread(),
"must be current thread or at safepoint");
You're looking at an outdated webrev, I'm afraid.
This would be the post with the current webrev.1
http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html
Sorry I missed that update. Okay so this is working with direct
handshakes now.
One style nit in jvmtiThreadState.cpp:
assert(SafepointSynchronize::is_at_safepoint() ||
! (JavaThread *)Thread::current() == get_thread() ||
! Thread::current() == get_thread()->active_handshaker(),
! "bad synchronization with owner thread");
the ! lines should ident as follows
assert(SafepointSynchronize::is_at_safepoint() ||
(JavaThread *)Thread::current() == get_thread() ||
Thread::current() == get_thread()->active_handshaker(),
! "bad synchronization with owner thread");
Lets see how this plays out.
Cheers,
David
Thanks, Richard.
-Original Message-
From: David Holmes
Sent: Montag, 4. Mai 2020 08:51
To: Reingruber, Richard ; Yasumasa Suenaga
; Patricio Chilano ;
serguei.spit...@oracle.com; Vladimir Ivanov ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 28/04/2020 12:09 am, Reingruber, Richard wrote:
Hi David,
Not a review but some general commentary ...
That's welcome.
Having had to take an even closer look now I have a review comment too :)
src/hotspot/share/prims/jvmtiThreadState.cpp
void JvmtiThreadState::invalidate_cur_stack_depth() {
! assert(SafepointSynchronize::is_at_safepoint() ||
! (Thread::current()->is_VM_thread() &&
get_thread()->is_vmthread_processing_handshake()) ||
(JavaThread *)Thread::current() == get_thread(),
"must be current thread or at safepoint");
The message needs updating to include handshakes.
More below ...
On 25/04/2020 2:08 am, Reingruber, Richard wrote:
Hi Yasumasa, Patricio,
I will send review request to replace VM_SetFramePop to
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 On Behalf Of
Reingruber, Richard
Sent: Freitag, 14. Februar 2020 19:47
To: Patricio Chilano ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@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(, 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
Sent: Freitag, 14. Februar 2020 15:54
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@openjdk.java.net
Subject: Re: RFR(S) 8238585: Use handshake for
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi David,
> On 4/05/2020 8:33 pm, Reingruber, Richard wrote:
> > // Trimmed the list of recipients. If the list gets too long then the
> > message needs to be approved
> > // by a moderator.
> Yes I noticed that too :) In general if you send to hotspot-dev you
> shouldn't need to also send to hotspot-X-dev.
Makes sense. Will do so next time.
> >
> > This would be the post with the current webrev.1
> >
> >
> > http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html
> Sorry I missed that update. Okay so this is working with direct
> handshakes now.
> One style nit in jvmtiThreadState.cpp:
> assert(SafepointSynchronize::is_at_safepoint() ||
> ! (JavaThread *)Thread::current() == get_thread() ||
> ! Thread::current() == get_thread()->active_handshaker(),
> ! "bad synchronization with owner thread");
> the ! lines should ident as follows
> assert(SafepointSynchronize::is_at_safepoint() ||
> (JavaThread *)Thread::current() == get_thread() ||
> Thread::current() == get_thread()->active_handshaker(),
> ! "bad synchronization with owner thread");
Sure.
> Lets see how this plays out.
Hopefully not too bad... :)
>> Not a review but some general commentary ...
Still not a review, or is it now?
Thanks, Richard.
-Original Message-
From: David Holmes
Sent: Mittwoch, 13. Mai 2020 07:43
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@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
On 4/05/2020 8:33 pm, Reingruber, Richard wrote:
> // Trimmed the list of recipients. If the list gets too long then the message
> needs to be approved
> // by a moderator.
Yes I noticed that too :) In general if you send to hotspot-dev you
shouldn't need to also send to hotspot-X-dev.
> Hi David,
Hi Richard,
>> On 28/04/2020 12:09 am, Reingruber, Richard wrote:
>>> Hi David,
>>>
Not a review but some general commentary ...
>>>
>>> That's welcome.
>
>> Having had to take an even closer look now I have a review comment too :)
>
>> src/hotspot/share/prims/jvmtiThreadState.cpp
>
>> void JvmtiThreadState::invalidate_cur_stack_depth() {
>> ! assert(SafepointSynchronize::is_at_safepoint() ||
>> ! (Thread::current()->is_VM_thread() &&
>> get_thread()->is_vmthread_processing_handshake()) ||
>> (JavaThread *)Thread::current() == get_thread(),
>> "must be current thread or at safepoint");
>
> You're looking at an outdated webrev, I'm afraid.
>
> This would be the post with the current webrev.1
>
>
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html
Sorry I missed that update. Okay so this is working with direct
handshakes now.
One style nit in jvmtiThreadState.cpp:
assert(SafepointSynchronize::is_at_safepoint() ||
! (JavaThread *)Thread::current() == get_thread() ||
! Thread::current() == get_thread()->active_handshaker(),
! "bad synchronization with owner thread");
the ! lines should ident as follows
assert(SafepointSynchronize::is_at_safepoint() ||
(JavaThread *)Thread::current() == get_thread() ||
Thread::current() == get_thread()->active_handshaker(),
! "bad synchronization with owner thread");
Lets see how this plays out.
Cheers,
David
> Thanks, Richard.
>
> -Original Message-
> From: David Holmes
> Sent: Montag, 4. Mai 2020 08:51
> To: Reingruber, Richard ; Yasumasa Suenaga
> ; Patricio Chilano
> ; serguei.spit...@oracle.com; Vladimir
> Ivanov ; serviceability-dev@openjdk.java.net;
> hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net;
> hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 28/04/2020 12:09 am, Reingruber, Richard wrote:
>> Hi David,
>>
>>> Not a review but some general commentary ...
>>
>> That's welcome.
>
> Having had to take an even closer look now I have a review comment too :)
>
> src/hotspot/share/prims/jvmtiThreadState.cpp
>
> void JvmtiThreadState::invalidate_cur_stack_depth() {
> ! assert(SafepointSynchronize::is_at_safepoint() ||
> ! (Thread::current()->is_VM_thread() &&
> get_thread()->is_vmthread_processing_handshake()) ||
> (JavaThread *)Thread::current() == get_thread(),
> "must be current thread or at safepoint");
>
> The message needs updating to include handshakes.
>
> More below ...
>
>>> On 25/04/2020 2:08 am, Reingruber, Richard wrote:
Hi Yasumasa, Patricio,
>>> I will send review request to replace
Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
On 4/05/2020 8:33 pm, Reingruber, Richard wrote:
// Trimmed the list of recipients. If the list gets too long then the message
needs to be approved
// by a moderator.
Yes I noticed that too :) In general if you send to hotspot-dev you
shouldn't need to also send to hotspot-X-dev.
Hi David,
Hi Richard,
On 28/04/2020 12:09 am, Reingruber, Richard wrote:
Hi David,
Not a review but some general commentary ...
That's welcome.
Having had to take an even closer look now I have a review comment too :)
src/hotspot/share/prims/jvmtiThreadState.cpp
void JvmtiThreadState::invalidate_cur_stack_depth() {
! assert(SafepointSynchronize::is_at_safepoint() ||
! (Thread::current()->is_VM_thread() &&
get_thread()->is_vmthread_processing_handshake()) ||
(JavaThread *)Thread::current() == get_thread(),
"must be current thread or at safepoint");
You're looking at an outdated webrev, I'm afraid.
This would be the post with the current webrev.1
http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html
Sorry I missed that update. Okay so this is working with direct
handshakes now.
One style nit in jvmtiThreadState.cpp:
assert(SafepointSynchronize::is_at_safepoint() ||
! (JavaThread *)Thread::current() == get_thread() ||
! Thread::current() == get_thread()->active_handshaker(),
! "bad synchronization with owner thread");
the ! lines should ident as follows
assert(SafepointSynchronize::is_at_safepoint() ||
(JavaThread *)Thread::current() == get_thread() ||
Thread::current() == get_thread()->active_handshaker(),
! "bad synchronization with owner thread");
Lets see how this plays out.
Cheers,
David
Thanks, Richard.
-Original Message-
From: David Holmes
Sent: Montag, 4. Mai 2020 08:51
To: Reingruber, Richard ; Yasumasa Suenaga
; Patricio Chilano ;
serguei.spit...@oracle.com; Vladimir Ivanov ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 28/04/2020 12:09 am, Reingruber, Richard wrote:
Hi David,
Not a review but some general commentary ...
That's welcome.
Having had to take an even closer look now I have a review comment too :)
src/hotspot/share/prims/jvmtiThreadState.cpp
void JvmtiThreadState::invalidate_cur_stack_depth() {
! assert(SafepointSynchronize::is_at_safepoint() ||
! (Thread::current()->is_VM_thread() &&
get_thread()->is_vmthread_processing_handshake()) ||
(JavaThread *)Thread::current() == get_thread(),
"must be current thread or at safepoint");
The message needs updating to include handshakes.
More below ...
On 25/04/2020 2:08 am, Reingruber, Richard wrote:
Hi Yasumasa, Patricio,
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.
Thanks :)
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.
Yes. To me it is unclear what synchronization is necessary, if it is called
during a handshake. And
also I'm unsure if a thread should do safepoint checks while executing a
handshake.
I'm growing increasingly concerned that use of direct handshakes to
replace VM operations needs a much greater examination for correctness
than might initially be thought. I see a number of issues:
I agree. I'll address your concerns in the context of this review thread for
JDK-8238585 below.
In addition I would suggest to take the general part of the discussion to a
dedicated thread or to
the review thread for JDK-8242427. I would like to keep this thread closer to
its subject.
I will focus on the issues in the context of this particular change
then, though
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Thanks Martin.
Cheers, Richard.
-Original Message-
From: Doerr, Martin
Sent: Dienstag, 12. Mai 2020 10:43
To: Reingruber, Richard ; David Holmes
; serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 had already reviewed webrev.1. Looks good to me.
Thanks for contributing it.
Best regards,
Martin
> -Original Message-
> From: hotspot-runtime-dev boun...@openjdk.java.net> On Behalf Of Reingruber, Richard
> Sent: Montag, 4. Mai 2020 12:33
> To: David Holmes ; serviceability-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-
> gc-...@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
>
> // Trimmed the list of recipients. If the list gets too long then the message
> needs to be approved
> // by a moderator.
>
> Hi David,
>
> > On 28/04/2020 12:09 am, Reingruber, Richard wrote:
> > > Hi David,
> > >
> > >> Not a review but some general commentary ...
> > >
> > > That's welcome.
>
> > Having had to take an even closer look now I have a review comment too :)
>
> > src/hotspot/share/prims/jvmtiThreadState.cpp
>
> >void JvmtiThreadState::invalidate_cur_stack_depth() {
> > ! assert(SafepointSynchronize::is_at_safepoint() ||
> > ! (Thread::current()->is_VM_thread() &&
> > get_thread()->is_vmthread_processing_handshake()) ||
> >(JavaThread *)Thread::current() == get_thread(),
> >"must be current thread or at safepoint");
>
> You're looking at an outdated webrev, I'm afraid.
>
> This would be the post with the current webrev.1
>
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-
> April/031245.html
>
> Thanks, Richard.
>
> -Original Message-
> From: David Holmes
> Sent: Montag, 4. Mai 2020 08:51
> To: Reingruber, Richard ; Yasumasa Suenaga
> ; Patricio Chilano
> ; serguei.spit...@oracle.com; Vladimir
> Ivanov ; serviceability-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-
> gc-...@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 28/04/2020 12:09 am, Reingruber, Richard wrote:
> > Hi David,
> >
> >> Not a review but some general commentary ...
> >
> > That's welcome.
>
> Having had to take an even closer look now I have a review comment too :)
>
> src/hotspot/share/prims/jvmtiThreadState.cpp
>
>void JvmtiThreadState::invalidate_cur_stack_depth() {
> ! assert(SafepointSynchronize::is_at_safepoint() ||
> ! (Thread::current()->is_VM_thread() &&
> get_thread()->is_vmthread_processing_handshake()) ||
>(JavaThread *)Thread::current() == get_thread(),
>"must be current thread or at safepoint");
>
> The message needs updating to include handshakes.
>
> More below ...
>
> >> On 25/04/2020 2:08 am, Reingruber, Richard wrote:
> >>> Hi Yasumasa, Patricio,
> >>>
> >> 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.
> >>>
> >>> Thanks :)
> >>>
> > 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.
> >>>
> >>> Yes. To me it is unclear what synchronization is necessary, if it is
> >>> called
> during a handshake. And
> >>> also I'm unsure if a thread should
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 had already reviewed webrev.1. Looks good to me.
Thanks for contributing it.
Best regards,
Martin
> -Original Message-
> From: hotspot-runtime-dev boun...@openjdk.java.net> On Behalf Of Reingruber, Richard
> Sent: Montag, 4. Mai 2020 12:33
> To: David Holmes ; serviceability-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-
> gc-...@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
>
> // Trimmed the list of recipients. If the list gets too long then the message
> needs to be approved
> // by a moderator.
>
> Hi David,
>
> > On 28/04/2020 12:09 am, Reingruber, Richard wrote:
> > > Hi David,
> > >
> > >> Not a review but some general commentary ...
> > >
> > > That's welcome.
>
> > Having had to take an even closer look now I have a review comment too :)
>
> > src/hotspot/share/prims/jvmtiThreadState.cpp
>
> >void JvmtiThreadState::invalidate_cur_stack_depth() {
> > ! assert(SafepointSynchronize::is_at_safepoint() ||
> > ! (Thread::current()->is_VM_thread() &&
> > get_thread()->is_vmthread_processing_handshake()) ||
> >(JavaThread *)Thread::current() == get_thread(),
> >"must be current thread or at safepoint");
>
> You're looking at an outdated webrev, I'm afraid.
>
> This would be the post with the current webrev.1
>
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-
> April/031245.html
>
> Thanks, Richard.
>
> -Original Message-
> From: David Holmes
> Sent: Montag, 4. Mai 2020 08:51
> To: Reingruber, Richard ; Yasumasa Suenaga
> ; Patricio Chilano
> ; serguei.spit...@oracle.com; Vladimir
> Ivanov ; serviceability-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-
> gc-...@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 28/04/2020 12:09 am, Reingruber, Richard wrote:
> > Hi David,
> >
> >> Not a review but some general commentary ...
> >
> > That's welcome.
>
> Having had to take an even closer look now I have a review comment too :)
>
> src/hotspot/share/prims/jvmtiThreadState.cpp
>
>void JvmtiThreadState::invalidate_cur_stack_depth() {
> ! assert(SafepointSynchronize::is_at_safepoint() ||
> ! (Thread::current()->is_VM_thread() &&
> get_thread()->is_vmthread_processing_handshake()) ||
>(JavaThread *)Thread::current() == get_thread(),
>"must be current thread or at safepoint");
>
> The message needs updating to include handshakes.
>
> More below ...
>
> >> On 25/04/2020 2:08 am, Reingruber, Richard wrote:
> >>> Hi Yasumasa, Patricio,
> >>>
> >> 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.
> >>>
> >>> Thanks :)
> >>>
> > 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.
> >>>
> >>> Yes. To me it is unclear what synchronization is necessary, if it is
> >>> called
> during a handshake. And
> >>> also I'm unsure if a thread should do safepoint checks while executing a
> handshake.
> >
> >> I'm growing increasingly concerned that use of direct handshakes to
> >> replace VM operations needs a much greater examination for correctness
> >> than might initially be thought. I see a number of issues:
> >
> > I agree. I'll address your concerns in the context of this review thread for
> JDK-8238585 below.
> >
> > In addition I would suggest to take the general part of the discussion to a
> dedicated thread or to
> > the
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 28/04/2020 12:09 am, Reingruber, Richard wrote:
Hi David,
Not a review but some general commentary ...
That's welcome.
Having had to take an even closer look now I have a review comment too :)
src/hotspot/share/prims/jvmtiThreadState.cpp
void JvmtiThreadState::invalidate_cur_stack_depth() {
! assert(SafepointSynchronize::is_at_safepoint() ||
! (Thread::current()->is_VM_thread() &&
get_thread()->is_vmthread_processing_handshake()) ||
(JavaThread *)Thread::current() == get_thread(),
"must be current thread or at safepoint");
The message needs updating to include handshakes.
More below ...
On 25/04/2020 2:08 am, Reingruber, Richard wrote:
Hi Yasumasa, Patricio,
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.
Thanks :)
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.
Yes. To me it is unclear what synchronization is necessary, if it is called
during a handshake. And
also I'm unsure if a thread should do safepoint checks while executing a
handshake.
I'm growing increasingly concerned that use of direct handshakes to
replace VM operations needs a much greater examination for correctness
than might initially be thought. I see a number of issues:
I agree. I'll address your concerns in the context of this review thread for
JDK-8238585 below.
In addition I would suggest to take the general part of the discussion to a
dedicated thread or to
the review thread for JDK-8242427. I would like to keep this thread closer to
its subject.
I will focus on the issues in the context of this particular change
then, though the issues themselves are applicable to all handshake
situations (and more so with direct handshakes). This is mostly just
discussion.
First, the VMThread executes (most) VM operations with a clean stack in
a clean state, so it has lots of room to work. If we now execute the
same logic in a JavaThread then we risk hitting stackoverflows if
nothing else. But we are also now executing code in a JavaThread and so
we have to be sure that code is not going to act differently (in a bad
way) if executed by a JavaThread rather than the VMThread. For example,
may it be possible that if executing in the VMThread we defer some
activity that might require execution of Java code, or else hand it off
to one of the service threads? If we execute that code directly in the
current JavaThread instead we may not be in a valid state (e.g. consider
re-entrancy to various subsystems that is not allowed).
It is not too complex, what EnterInterpOnlyModeClosure::do_thread() is doing. I
already added a
paragraph to the JBS-Item [1] explaining why the direct handshake is sufficient
from a
synchronization point of view.
Just to be clear, your proposed change is not using a direct handshake.
Furthermore the stack is walked and the return pc of compiled frames is
replaced with the address of
the deopt handler.
I can't see why this cannot be done with a direct handshake. Something very
similar is already done
in JavaThread::deoptimize_marked_methods() which is executed as part of an
ordinary handshake.
Note that existing non-direct handshakes may also have issues that not
have been fully investigated.
The demand on stack-space should be very modest. I would not expect a higher
risk for stackoverflow.
For the target thread if you use more stack than would be used stopping
at a safepoint then you are at risk. For the thread initiating the
direct handshake if you use more stack than would be used enqueuing a VM
operation, then you are at risk. As we have not quantified these
numbers, nor have any easy way to establish the stack use of the actual
code to be executed, we're really just hoping for the best. This is a
general problem with handshakes that needs to be investigated more
deeply. As a simple, general, example just imagine if the code involves
logging
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
// Trimmed the list of recipients. If the list gets too long then the message
needs to be approved
// by a moderator.
Hi David,
> On 28/04/2020 12:09 am, Reingruber, Richard wrote:
> > Hi David,
> >
> >> Not a review but some general commentary ...
> >
> > That's welcome.
> Having had to take an even closer look now I have a review comment too :)
> src/hotspot/share/prims/jvmtiThreadState.cpp
>void JvmtiThreadState::invalidate_cur_stack_depth() {
> ! assert(SafepointSynchronize::is_at_safepoint() ||
> ! (Thread::current()->is_VM_thread() &&
> get_thread()->is_vmthread_processing_handshake()) ||
>(JavaThread *)Thread::current() == get_thread(),
>"must be current thread or at safepoint");
You're looking at an outdated webrev, I'm afraid.
This would be the post with the current webrev.1
http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html
Thanks, Richard.
-Original Message-
From: David Holmes
Sent: Montag, 4. Mai 2020 08:51
To: Reingruber, Richard ; Yasumasa Suenaga
; Patricio Chilano
; serguei.spit...@oracle.com; Vladimir
Ivanov ; serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 28/04/2020 12:09 am, Reingruber, Richard wrote:
> Hi David,
>
>> Not a review but some general commentary ...
>
> That's welcome.
Having had to take an even closer look now I have a review comment too :)
src/hotspot/share/prims/jvmtiThreadState.cpp
void JvmtiThreadState::invalidate_cur_stack_depth() {
! assert(SafepointSynchronize::is_at_safepoint() ||
! (Thread::current()->is_VM_thread() &&
get_thread()->is_vmthread_processing_handshake()) ||
(JavaThread *)Thread::current() == get_thread(),
"must be current thread or at safepoint");
The message needs updating to include handshakes.
More below ...
>> On 25/04/2020 2:08 am, Reingruber, Richard wrote:
>>> Hi Yasumasa, Patricio,
>>>
>> 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.
>>>
>>> Thanks :)
>>>
> 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.
>>>
>>> Yes. To me it is unclear what synchronization is necessary, if it is called
>>> during a handshake. And
>>> also I'm unsure if a thread should do safepoint checks while executing a
>>> handshake.
>
>> I'm growing increasingly concerned that use of direct handshakes to
>> replace VM operations needs a much greater examination for correctness
>> than might initially be thought. I see a number of issues:
>
> I agree. I'll address your concerns in the context of this review thread for
> JDK-8238585 below.
>
> In addition I would suggest to take the general part of the discussion to a
> dedicated thread or to
> the review thread for JDK-8242427. I would like to keep this thread closer to
> its subject.
I will focus on the issues in the context of this particular change
then, though the issues themselves are applicable to all handshake
situations (and more so with direct handshakes). This is mostly just
discussion.
>> First, the VMThread executes (most) VM operations with a clean stack in
>> a clean state, so it has lots of room to work. If we now execute the
>> same logic in a JavaThread then we risk hitting stackoverflows if
>> nothing else. But we are also now executing code in a JavaThread and so
>> we have to be sure that code is not going to act differently (in a bad
>> way) if executed by a JavaThread rather than the VMThread. For example,
>> may it be possible that if
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi David, > Not a review but some general commentary ... That's welcome. > On 25/04/2020 2:08 am, Reingruber, Richard wrote: > > Hi Yasumasa, Patricio, > > > 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. > > > > Thanks :) > > > >>> 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. > > > > Yes. To me it is unclear what synchronization is necessary, if it is called > > during a handshake. And > > also I'm unsure if a thread should do safepoint checks while executing a > > handshake. > I'm growing increasingly concerned that use of direct handshakes to > replace VM operations needs a much greater examination for correctness > than might initially be thought. I see a number of issues: I agree. I'll address your concerns in the context of this review thread for JDK-8238585 below. In addition I would suggest to take the general part of the discussion to a dedicated thread or to the review thread for JDK-8242427. I would like to keep this thread closer to its subject. > First, the VMThread executes (most) VM operations with a clean stack in > a clean state, so it has lots of room to work. If we now execute the > same logic in a JavaThread then we risk hitting stackoverflows if > nothing else. But we are also now executing code in a JavaThread and so > we have to be sure that code is not going to act differently (in a bad > way) if executed by a JavaThread rather than the VMThread. For example, > may it be possible that if executing in the VMThread we defer some > activity that might require execution of Java code, or else hand it off > to one of the service threads? If we execute that code directly in the > current JavaThread instead we may not be in a valid state (e.g. consider > re-entrancy to various subsystems that is not allowed). It is not too complex, what EnterInterpOnlyModeClosure::do_thread() is doing. I already added a paragraph to the JBS-Item [1] explaining why the direct handshake is sufficient from a synchronization point of view. Furthermore the stack is walked and the return pc of compiled frames is replaced with the address of the deopt handler. I can't see why this cannot be done with a direct handshake. Something very similar is already done in JavaThread::deoptimize_marked_methods() which is executed as part of an ordinary handshake. The demand on stack-space should be very modest. I would not expect a higher risk for stackoverflow. > Second, we have this question mark over what happens if the operation > hits further safepoint or handshake polls/checks? Are there constraints > on what is allowed here? How can we recognise this problem may exist and > so deal with it? The thread in EnterInterpOnlyModeClosure::do_thread() can't become safepoint/handshake safe. I tested locally test/hotspot/jtreg:vmTestbase_nsk_jvmti with a NoSafepointVerifier. > Third, while we are generally considering what appear to be > single-thread operations, which should be amenable to a direct > handshake, we also have to be careful that some of the code involved > doesn't already expect/assume we are at a safepoint - e.g. a VM op may > not need to take a lock where a direct handshake might! See again my arguments in the JBS item [1]. Thanks, Richard. [1] https://bugs.openjdk.java.net/browse/JDK-8238585 -Original Message- From: David Holmes Sent: Montag, 27. April 2020 07:16 To: Reingruber, Richard ; Yasumasa Suenaga ; Patricio Chilano ; serguei.spit...@oracle.com; Vladimir Ivanov ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net Subject: Re: RFR(S) 8238585: Use handshake for
Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi all, Not a review but some general commentary ... On 25/04/2020 2:08 am, Reingruber, Richard wrote: Hi Yasumasa, Patricio, 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. Thanks :) 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. Yes. To me it is unclear what synchronization is necessary, if it is called during a handshake. And also I'm unsure if a thread should do safepoint checks while executing a handshake. I'm growing increasingly concerned that use of direct handshakes to replace VM operations needs a much greater examination for correctness than might initially be thought. I see a number of issues: First, the VMThread executes (most) VM operations with a clean stack in a clean state, so it has lots of room to work. If we now execute the same logic in a JavaThread then we risk hitting stackoverflows if nothing else. But we are also now executing code in a JavaThread and so we have to be sure that code is not going to act differently (in a bad way) if executed by a JavaThread rather than the VMThread. For example, may it be possible that if executing in the VMThread we defer some activity that might require execution of Java code, or else hand it off to one of the service threads? If we execute that code directly in the current JavaThread instead we may not be in a valid state (e.g. consider re-entrancy to various subsystems that is not allowed). Second, we have this question mark over what happens if the operation hits further safepoint or handshake polls/checks? Are there constraints on what is allowed here? How can we recognise this problem may exist and so deal with it? Third, while we are generally considering what appear to be single-thread operations, which should be amenable to a direct handshake, we also have to be careful that some of the code involved doesn't already expect/assume we are at a safepoint - e.g. a VM op may not need to take a lock where a direct handshake might! Cheers, David - @Patricio, coming back to my question [1]: In the example you gave in your answer [2]: the java thread would execute a vm operation during a direct handshake operation, while the VMThread is actually in the middle of a VM_HandshakeAllThreads operation, waiting to handshake the same handshakee: why can't the VMThread just proceed? The handshakee would be safepoint safe, wouldn't it? Thanks, Richard. [1] https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301677=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301677 [2] https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301763=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301763 -Original Message- From: Yasumasa Suenaga Sent: Freitag, 24. April 2020 17:23 To: Reingruber, Richard ; Patricio Chilano ; serguei.spit...@oracle.com; Vladimir Ivanov ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 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
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi Patricio, thanks a lot for all the explanations. At least to me they are really helpful. :) Cheers, Richard. -Original Message- From: Patricio Chilano Sent: Samstag, 25. April 2020 11:23 To: Reingruber, Richard ; Yasumasa Suenaga ; serguei.spit...@oracle.com; Vladimir Ivanov ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 4/24/20 6:41 PM, Reingruber, Richard wrote: > Hi Patricio, > >>> @Patricio, coming back to my question [1]: >>> >>> In the example you gave in your answer [2]: the java thread would execute a >>> vm operation during a >>> direct handshake operation, while the VMThread is actually in the middle of >>> a VM_HandshakeAllThreads >>> operation, waiting to handshake the same handshakee: why can't the VMThread >>> just proceed? The >>> handshakee would be safepoint safe, wouldn't it? >> Because the VMThread would not be able to decrement _processing_sem to >> claim the operation and execute the closure for that handshakee. If >> another JavaThread is doing a direct handshake with that same handshakee >> and called a new VM operation inside the execution of the >> HandshakeClosure in do_handshake(), nobody would be able to decrement >> the _processing_sem anymore until the original direct operation finished >> and the semaphore is signaled again. > Thanks, understood. On a higher level: a JavaThread can have at most one > handshake operation being > processed at at time. Exactly. As of now we don't handle the case where another handshake operation on the same handshakee is called inside _handshake_cl->do_thread(). If this happens we will deadlock. >> So this can happen despite the >> state of the handshakee is "handshake/safepoint safe". Changing the >> nested VM operation to be a direct handshake would have the same issue. >> Actually as the code is right now we would not even get pass setting the >> handshake operation because in that case we would block in the >> _handshake_turn_sem for the same reason. > Don't really understand the details here, but that's ok. > Interesting that _handshake_turn_sem gets signaled before or after > do_handshake() depending if the > handshake operation is processed by handshakee. Comments say "Disarm > before/after executing > operation" but not why :) Yes, that pattern actually relates with clearing _operation and predates direct handshakes. In theory we should always call do_handshake() first and then clear the handshake. This is what we do when the operation is processed by the handshaker, and it is necessary to be that way, otherwise if we clear the handshake first then the handshakee might transition from the safe state and never see that it actually has to stop for the handshake. Now, when the handshake operation is processed by the handshakee itself we don't have that issue, so it doesn't matter if we clear it before or after. The reason we do it before is to avoid the VMThread to execute unnecessary instructions in try_process(). This is specially true for the VM_HandshakeAllThreads operation case. If the VMThread sees that a JavaThread doesn't have an operation set, it can just continue to try to process the next JavaThread, instead of going through the unnecessary steps of checking the state of the JavaThread and trying to execute a try_wait() operation on the _processing_sem which we know won't succeed. Now for the direct handshake case doing it before or after doesn't really matter and so I just copied the pattern from the non-direct case to make it consistent in that same method. >> So changing VM_SetFramePop to use direct handshakes in the future will >> probably create that last issue I mentioned. Now, since it is executed >> at a safepoint, with your workaround in enter_interp_only_mode() we >> avoid those nested issues in . Maybe 8239084 would have to be revisited >> to address nested operations in all cases. It is not clear to me now >> though if we should handle that in the handshake code or the caller of a >> certain operation should know it might be called in a nested scenario >> and should handle it. > Last question: is it ok for the processor of a direct handshake operation to > do safepoint/handshake > checks? I wouldn't see a reason, why not. But certainly I would avoid it. I tried to think of possible issues with that (independent of the closure logic) but I couldn't find a specific one. If the handshakee tries to process a pending handshake, process_by_self() will just return without calling process_self_inner() since it will detect it is already inside a handshake. And that behaviour makes sense since there is no point in trying to execute a new handshake operation if you are in the
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 4/24/20 6:41 PM, Reingruber, Richard wrote: Hi Patricio, @Patricio, coming back to my question [1]: In the example you gave in your answer [2]: the java thread would execute a vm operation during a direct handshake operation, while the VMThread is actually in the middle of a VM_HandshakeAllThreads operation, waiting to handshake the same handshakee: why can't the VMThread just proceed? The handshakee would be safepoint safe, wouldn't it? Because the VMThread would not be able to decrement _processing_sem to claim the operation and execute the closure for that handshakee. If another JavaThread is doing a direct handshake with that same handshakee and called a new VM operation inside the execution of the HandshakeClosure in do_handshake(), nobody would be able to decrement the _processing_sem anymore until the original direct operation finished and the semaphore is signaled again. Thanks, understood. On a higher level: a JavaThread can have at most one handshake operation being processed at at time. Exactly. As of now we don't handle the case where another handshake operation on the same handshakee is called inside _handshake_cl->do_thread(). If this happens we will deadlock. So this can happen despite the state of the handshakee is "handshake/safepoint safe". Changing the nested VM operation to be a direct handshake would have the same issue. Actually as the code is right now we would not even get pass setting the handshake operation because in that case we would block in the _handshake_turn_sem for the same reason. Don't really understand the details here, but that's ok. Interesting that _handshake_turn_sem gets signaled before or after do_handshake() depending if the handshake operation is processed by handshakee. Comments say "Disarm before/after executing operation" but not why :) Yes, that pattern actually relates with clearing _operation and predates direct handshakes. In theory we should always call do_handshake() first and then clear the handshake. This is what we do when the operation is processed by the handshaker, and it is necessary to be that way, otherwise if we clear the handshake first then the handshakee might transition from the safe state and never see that it actually has to stop for the handshake. Now, when the handshake operation is processed by the handshakee itself we don't have that issue, so it doesn't matter if we clear it before or after. The reason we do it before is to avoid the VMThread to execute unnecessary instructions in try_process(). This is specially true for the VM_HandshakeAllThreads operation case. If the VMThread sees that a JavaThread doesn't have an operation set, it can just continue to try to process the next JavaThread, instead of going through the unnecessary steps of checking the state of the JavaThread and trying to execute a try_wait() operation on the _processing_sem which we know won't succeed. Now for the direct handshake case doing it before or after doesn't really matter and so I just copied the pattern from the non-direct case to make it consistent in that same method. So changing VM_SetFramePop to use direct handshakes in the future will probably create that last issue I mentioned. Now, since it is executed at a safepoint, with your workaround in enter_interp_only_mode() we avoid those nested issues in . Maybe 8239084 would have to be revisited to address nested operations in all cases. It is not clear to me now though if we should handle that in the handshake code or the caller of a certain operation should know it might be called in a nested scenario and should handle it. Last question: is it ok for the processor of a direct handshake operation to do safepoint/handshake checks? I wouldn't see a reason, why not. But certainly I would avoid it. I tried to think of possible issues with that (independent of the closure logic) but I couldn't find a specific one. If the handshakee tries to process a pending handshake, process_by_self() will just return without calling process_self_inner() since it will detect it is already inside a handshake. And that behaviour makes sense since there is no point in trying to execute a new handshake operation if you are in the middle of another one. If the handshaker inside the closure checks for its own pending handshakes that also seems okay (this will by itself also check for safepoints in the transitions). Checking for safepoints in both cases seems more tricky but I couldn't think of a concrete issue with that. In any case I would also avoid checking for safepoints/handshakes inside the handshake closure. You might get issues related to the actual logic of the closure, like the typical deadlock because of trying to grab the same lock (although it's true that you always have to deal with that kind of problems when checking for safepoint/handshakes), or coming back from the safepoint/handshake and failing because some state you didn't expect to
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi Patricio, > > @Patricio, coming back to my question [1]: > > > > In the example you gave in your answer [2]: the java thread would execute a > > vm operation during a > > direct handshake operation, while the VMThread is actually in the middle of > > a VM_HandshakeAllThreads > > operation, waiting to handshake the same handshakee: why can't the VMThread > > just proceed? The > > handshakee would be safepoint safe, wouldn't it? > Because the VMThread would not be able to decrement _processing_sem to > claim the operation and execute the closure for that handshakee. If > another JavaThread is doing a direct handshake with that same handshakee > and called a new VM operation inside the execution of the > HandshakeClosure in do_handshake(), nobody would be able to decrement > the _processing_sem anymore until the original direct operation finished > and the semaphore is signaled again. Thanks, understood. On a higher level: a JavaThread can have at most one handshake operation being processed at at time. > So this can happen despite the > state of the handshakee is "handshake/safepoint safe". Changing the > nested VM operation to be a direct handshake would have the same issue. > Actually as the code is right now we would not even get pass setting the > handshake operation because in that case we would block in the > _handshake_turn_sem for the same reason. Don't really understand the details here, but that's ok. Interesting that _handshake_turn_sem gets signaled before or after do_handshake() depending if the handshake operation is processed by handshakee. Comments say "Disarm before/after executing operation" but not why :) > So changing VM_SetFramePop to use direct handshakes in the future will > probably create that last issue I mentioned. Now, since it is executed > at a safepoint, with your workaround in enter_interp_only_mode() we > avoid those nested issues in . Maybe 8239084 would have to be revisited > to address nested operations in all cases. It is not clear to me now > though if we should handle that in the handshake code or the caller of a > certain operation should know it might be called in a nested scenario > and should handle it. Last question: is it ok for the processor of a direct handshake operation to do safepoint/handshake checks? I wouldn't see a reason, why not. But certainly I would avoid it. > I'll look a bit more at the updated patch but at first glance looks good. Thanks, Richard. -Original Message- From: Patricio Chilano Sent: Freitag, 24. April 2020 19:14 To: Reingruber, Richard ; Yasumasa Suenaga ; serguei.spit...@oracle.com; Vladimir Ivanov ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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, Just jumping into your last question for now. : ) On 4/24/20 1:08 PM, Reingruber, Richard wrote: > Hi Yasumasa, Patricio, > 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. > Thanks :) > >>> 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. > Yes. To me it is unclear what synchronization is necessary, if it is called > during a handshake. And > also I'm unsure if a thread should do safepoint checks while executing a > handshake. > > @Patricio, coming back to my question [1]: > > In the example you gave in your answer [2]: the java thread would execute a > vm operation during a > direct handshake operation, while the VMThread is actually in the middle of a > VM_HandshakeAllThreads > operation, waiting to handshake the same handshakee: why can't the VMThread > just proceed? The >
Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi Richard, Just jumping into your last question for now. : ) On 4/24/20 1:08 PM, Reingruber, Richard wrote: Hi Yasumasa, Patricio, 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. Thanks :) 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. Yes. To me it is unclear what synchronization is necessary, if it is called during a handshake. And also I'm unsure if a thread should do safepoint checks while executing a handshake. @Patricio, coming back to my question [1]: In the example you gave in your answer [2]: the java thread would execute a vm operation during a direct handshake operation, while the VMThread is actually in the middle of a VM_HandshakeAllThreads operation, waiting to handshake the same handshakee: why can't the VMThread just proceed? The handshakee would be safepoint safe, wouldn't it? Because the VMThread would not be able to decrement _processing_sem to claim the operation and execute the closure for that handshakee. If another JavaThread is doing a direct handshake with that same handshakee and called a new VM operation inside the execution of the HandshakeClosure in do_handshake(), nobody would be able to decrement the _processing_sem anymore until the original direct operation finished and the semaphore is signaled again. So this can happen despite the state of the handshakee is "handshake/safepoint safe". Changing the nested VM operation to be a direct handshake would have the same issue. Actually as the code is right now we would not even get pass setting the handshake operation because in that case we would block in the _handshake_turn_sem for the same reason. So changing VM_SetFramePop to use direct handshakes in the future will probably create that last issue I mentioned. Now, since it is executed at a safepoint, with your workaround in enter_interp_only_mode() we avoid those nested issues in . Maybe 8239084 would have to be revisited to address nested operations in all cases. It is not clear to me now though if we should handle that in the handshake code or the caller of a certain operation should know it might be called in a nested scenario and should handle it. I'll look a bit more at the updated patch but at first glance looks good. Thanks! Patricio Thanks, Richard. [1] https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301677=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301677 [2] https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301763=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301763 -Original Message- From: Yasumasa Suenaga Sent: Freitag, 24. April 2020 17:23 To: Reingruber, Richard ; Patricio Chilano ; serguei.spit...@oracle.com; Vladimir Ivanov ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 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
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi Yasumasa, Patricio, > >> 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. Thanks :) > > 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. Yes. To me it is unclear what synchronization is necessary, if it is called during a handshake. And also I'm unsure if a thread should do safepoint checks while executing a handshake. @Patricio, coming back to my question [1]: In the example you gave in your answer [2]: the java thread would execute a vm operation during a direct handshake operation, while the VMThread is actually in the middle of a VM_HandshakeAllThreads operation, waiting to handshake the same handshakee: why can't the VMThread just proceed? The handshakee would be safepoint safe, wouldn't it? Thanks, Richard. [1] https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301677=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301677 [2] https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301763=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301763 -Original Message- From: Yasumasa Suenaga Sent: Freitag, 24. April 2020 17:23 To: Reingruber, Richard ; Patricio Chilano ; serguei.spit...@oracle.com; Vladimir Ivanov ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 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=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14302030 > > -Original Message- > From: Yasumasa Suenaga > Sent: Freitag, 24. April 2020 13:34 > To: Reingruber, Richard ; Patricio Chilano > ; serguei.spit...@oracle.com; Vladimir > Ivanov ; serviceability-dev@openjdk.java.net; > hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; > hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net > Subject: Re: RFR(S)
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 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=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14302030
-Original Message-
From: Yasumasa Suenaga
Sent: Freitag, 24. April 2020 13:34
To: Reingruber, Richard ; Patricio Chilano
; serguei.spit...@oracle.com; Vladimir Ivanov
; serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 On Behalf Of
Reingruber, Richard
Sent: Freitag, 14. Februar 2020 19:47
To: Patricio Chilano ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@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(, state->get_thread());
> >>}
> >> (you could pass “EnterInterpOnlyModeClosure” directly to the
> >> HandshakeClosure() constructor)
> >
> >
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
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.
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.
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=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14302030
-Original Message-
From: Yasumasa Suenaga
Sent: Freitag, 24. April 2020 13:34
To: Reingruber, Richard ; Patricio Chilano
; serguei.spit...@oracle.com; Vladimir
Ivanov ; serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@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 On Behalf Of
> Reingruber, Richard
> Sent: Freitag, 14. Februar 2020 19:47
> To: Patricio Chilano ;
> serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
> hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
> hotspot-gc-...@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(, 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
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 On Behalf Of
Reingruber, Richard
Sent: Freitag, 14. Februar 2020 19:47
To: Patricio Chilano ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@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(, 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
Sent: Freitag, 14. Februar 2020 15:54
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@openjdk.java.net
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
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 On Behalf Of
Reingruber, Richard
Sent: Freitag, 14. Februar 2020 19:47
To: Patricio Chilano ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@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(, 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
Sent: Freitag, 14. Februar 2020 15:54
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@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
>>
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(, 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
Sent: Freitag, 14. Februar 2020 15:54
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@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(, state->get_thread());
>
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(, 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
Sent: Donnerstag, 13. Februar 2020 18:47
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@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
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
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(, 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
Sent: Donnerstag, 13. Februar 2020 18:47
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@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(, 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
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(, 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
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Ok. I will repost and include hotspot runtime and gc lists. Thanks, Richard. -Original Message- From: Dean Long Sent: Dienstag, 11. Februar 2020 18:28 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@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 You might want to have some runtime/GC folks look at the handshake changes. dl On 2/6/20 4:39 AM, Reingruber, Richard wrote: > Hi, > > could I please get reviews for this small enhancement: > > 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
Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
You might want to have some runtime/GC folks look at the handshake changes. dl On 2/6/20 4:39 AM, Reingruber, Richard wrote: Hi, could I please get reviews for this small enhancement: 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
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi Serguei,
> Two reviews has to be good enough unless anyone else did not want to
> review it as well.
> I guess, it is good to push.
Ok. I'll wait a little longer and on Thursday I'll push it.
Thanks, Richard.
-Original Message-
From: serguei.spit...@oracle.com
Sent: Montag, 10. Februar 2020 19:11
To: Reingruber, Richard ; Vladimir Ivanov
; serviceability-dev@openjdk.java.net;
hotspot-compiler-...@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 details on testing!
Two reviews has to be good enough unless anyone else did not want to
review it as well.
I guess, it is good to push.
Thanks,
Serguei
On 2/10/20 03:26, Reingruber, Richard wrote:
> Hi Vladimir and Serguei,
>
> thanks for looking at the change!
>
>> What exact tests do you run to verify the fix?
>
> The enhancement was tested running the JCK and JTREG tests which include many
> JVMTI, JDI and JDWP tests.
>
> To see if the tests cover this part of the JVMTI implementation I had removed
> the deoptimization of
> compiled frames on stack. I found that e.g. the following test covers this:
>
>vmTestbase/nsk/jvmti/scenarios/events/EM02/em02t012
>
> The test
>
>vmTestbase/nsk/jvmti/scenarios/hotswap/HS202/hs202t002/hs202t002.java
>
> triggers the guarantee
>
> 238 void JvmtiThreadState::invalidate_cur_stack_depth() {
> 239 guarantee(SafepointSynchronize::is_at_safepoint() ||
> 240 (JavaThread *)Thread::current() == get_thread(),
> 241 "must be current thread or at safepoint");
> 242
> 243 _cur_stack_depth = UNKNOWN_STACK_DEPTH;
> 244 }
> 245
>
> because with the enhancement invalidate_cur_stack_depth() gets called by the
> VMThread executing the
> new handshake. So this is covered as well.
>
> Thanks again for reviewing.
>
> Do I need more reviews or are your reviews enough to push the enhancement?
>
> Best regards,
> Richard.
>
> -Original Message-
> From: serguei.spit...@oracle.com
> Sent: Freitag, 7. Februar 2020 19:06
> To: Reingruber, Richard ;
> serviceability-dev@openjdk.java.net; hotspot-compiler-...@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,
>
> It looks good to me.
> I can't comment on compiled methods non-entrancy.
>
> What exact tests do you run to verify the fix?
>
> Thanks,
> Serguei
>
>
> On 2/6/20 04:39, Reingruber, Richard wrote:
>> Hi,
>>
>> could I please get reviews for this small enhancement:
>>
>> 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
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 details on testing!
Two reviews has to be good enough unless anyone else did not want to
review it as well.
I guess, it is good to push.
Thanks,
Serguei
On 2/10/20 03:26, Reingruber, Richard wrote:
Hi Vladimir and Serguei,
thanks for looking at the change!
> What exact tests do you run to verify the fix?
The enhancement was tested running the JCK and JTREG tests which include many
JVMTI, JDI and JDWP tests.
To see if the tests cover this part of the JVMTI implementation I had removed
the deoptimization of
compiled frames on stack. I found that e.g. the following test covers this:
vmTestbase/nsk/jvmti/scenarios/events/EM02/em02t012
The test
vmTestbase/nsk/jvmti/scenarios/hotswap/HS202/hs202t002/hs202t002.java
triggers the guarantee
238 void JvmtiThreadState::invalidate_cur_stack_depth() {
239 guarantee(SafepointSynchronize::is_at_safepoint() ||
240 (JavaThread *)Thread::current() == get_thread(),
241 "must be current thread or at safepoint");
242
243 _cur_stack_depth = UNKNOWN_STACK_DEPTH;
244 }
245
because with the enhancement invalidate_cur_stack_depth() gets called by the
VMThread executing the
new handshake. So this is covered as well.
Thanks again for reviewing.
Do I need more reviews or are your reviews enough to push the enhancement?
Best regards,
Richard.
-Original Message-
From: serguei.spit...@oracle.com
Sent: Freitag, 7. Februar 2020 19:06
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@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,
It looks good to me.
I can't comment on compiled methods non-entrancy.
What exact tests do you run to verify the fix?
Thanks,
Serguei
On 2/6/20 04:39, Reingruber, Richard wrote:
Hi,
could I please get reviews for this small enhancement:
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
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi Vladimir and Serguei,
thanks for looking at the change!
> What exact tests do you run to verify the fix?
The enhancement was tested running the JCK and JTREG tests which include many
JVMTI, JDI and JDWP tests.
To see if the tests cover this part of the JVMTI implementation I had removed
the deoptimization of
compiled frames on stack. I found that e.g. the following test covers this:
vmTestbase/nsk/jvmti/scenarios/events/EM02/em02t012
The test
vmTestbase/nsk/jvmti/scenarios/hotswap/HS202/hs202t002/hs202t002.java
triggers the guarantee
238 void JvmtiThreadState::invalidate_cur_stack_depth() {
239 guarantee(SafepointSynchronize::is_at_safepoint() ||
240 (JavaThread *)Thread::current() == get_thread(),
241 "must be current thread or at safepoint");
242
243 _cur_stack_depth = UNKNOWN_STACK_DEPTH;
244 }
245
because with the enhancement invalidate_cur_stack_depth() gets called by the
VMThread executing the
new handshake. So this is covered as well.
Thanks again for reviewing.
Do I need more reviews or are your reviews enough to push the enhancement?
Best regards,
Richard.
-Original Message-
From: serguei.spit...@oracle.com
Sent: Freitag, 7. Februar 2020 19:06
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@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,
It looks good to me.
I can't comment on compiled methods non-entrancy.
What exact tests do you run to verify the fix?
Thanks,
Serguei
On 2/6/20 04:39, Reingruber, Richard wrote:
> Hi,
>
> could I please get reviews for this small enhancement:
>
> 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
Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi Richard, It looks good to me. I can't comment on compiled methods non-entrancy. What exact tests do you run to verify the fix? Thanks, Serguei On 2/6/20 04:39, Reingruber, Richard wrote: Hi, could I please get reviews for this small enhancement: 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
Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. Best regards, Vladimir Ivanov 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
