Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, This is on my review list. I'll try to get it reviewed by the end of this week. Thanks, Serguei On 9/8/20 10:02, Reingruber, Richard wrote: Hello Marty, Sure. I'd be happy if Serguei could review the change. Thanks, Richard. -Original Message- From: Marty Thompson Sent: Dienstag, 8. September 2020 18:55 To: Reingruber, Richard ; Daniel Daugherty ; serviceability-dev ; hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hello Richard, It would be good if Serguei Spitsyn could review before this is pushed. Serguei is out this week. Can you wait until Serguei is back in the office the week of Sept 14? Regards, Marty -Original Message- From: Reingruber, Richard Sent: Tuesday, September 8, 2020 9:45 AM To: Daniel Daugherty ; serviceability-dev ; hotspot-compiler- d...@openjdk.java.net; Hotspot dev runtime Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Dan, I'd be very happy about a review from somebody on the Serviceability team. I have asked for reviews many times (kindly I hope). And the change is for review for more than a year now. According to [1] I'd think all requirements to push are met already. But maybe I missed something? After renaming of methods in SafepointMechanism the change needs to be rebased (already done). I'll publish a pull request as soon as possible. Thanks, Richard. [1] https://wiki.openjdk.java.net/display/HotSpot/Pushing+a+HotSpot+change -Original Message- From: Daniel D. Daugherty Sent: Dienstag, 8. September 2020 18:16 To: Reingruber, Richard ; serviceability-dev ; hotspot-compiler- d...@openjdk.java.net; Hotspot dev runtime Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, I haven't seen a review from anyone on the Serviceability team and I think you should get a review from them since JVM/TI is involved. Perhaps I missed it... Dan On 9/7/20 10:09 AM, Reingruber, Richard wrote: Hi, I would like to close the review of this change. It has received a lot of helpful feedback during the process and 2 full Reviews. Thanks everybody! I'm planning to push it this week on Thursday as solution for JBS items: https://bugs.openjdk.java.net/browse/JDK-8227745 https://bugs.openjdk.java.net/browse/JDK-8233915 Version to be pushed: http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ Hope to get my GIT/Skara setup going until then... :) Thanks, Richard. -Original Message- From: hotspot-compiler-dev On Behalf Of Reingruber, Richard Sent: Mittwoch, 2. September 2020 23:27 To: Robbin Ehn ; serviceability-dev ; hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime Subject: [CAUTION] RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Robin, On 2020-09-02 15:48, Reingruber, Richard wrote: Hi Robbin, // taking the discussion back to the mailing lists > I still don't understand why you don't deoptimize the objects inside the > handshake/safepoint instead? So for handshakes using asynch handshake and allowing blocking inside would fix that. (future fix, I'm working on that now) Just to make it clear: I'm not fond of the extra suspension mechanism currently used for JDK-8227745 either. I want to get rid of it and I will work on it. Asynch handshakes (JDK-8238761) could be a replacement for it. At least I think they can be used to suspend the target thread. For safepoint, since we have suspended all threads, ~'safepointed them' with a JavaThread, you _could_ just execute the action directly (e.g. skipping VM_HeapWalkOperation safepoint) since they are suppose to be safely suspended until the destructor of EB, no? Yes, this should be possible. This would be an advanced change though. I would like EscapeBarriers to be a no-op and fall back to current implementation, if C2-EscapeAnalysis/Graal are disabled. So I suggest future work to instead just execute the safepoint with the requesting JT instead of having a this special safepoiting mechanism. Since you are missing above functionality I see why you went this way. If you need to push it, it's fine by me. We will work on further improvements. Top of the list would be eliminating the extra suspend mechanism. The implementation has matured for more than 12 months now [1]. It's been tested extensively at SAP over that time and passed also extended testing at Oracle kindly conducted by Vladimir Kozlov. We've got two full Reviews and incorporated extensive feedback from a number of OpenJDK Reviewers (including you, thanks!). Based on that I reckon we're good to push the change as enhancement (JDK-8227745) and bug fix (JDK-8233915). Thanks fo
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
> Hi Richard, > I suspect this one fell off the radar due to the extended review period. > The actual review started last December (there was prior discussion > IIRC) and only seemed to get partial reviews. I only looked at some > parts. Robbin may have given things a deeper look, but seemed focused on > the handshake aspects. Vladimir said he would do a full review but I > can't find it. Eventually Martin and Goetz took over reviewing and > everyone else dropped off. :( That's how it went I reckon. I repeatedly asked for feedback and reviews, and also tried to keep Vladimir, Robbin, and you in the loop addressing you directly (e.g. [1]) > As this covers a number of areas it really does need "approval" from > each area (and yes the hotspot wiki should reflect this). I agree. The wiki should define that in a clear manner. And the community should be involved in that definition. > I will try to take another look while we await Serguei's return (and I > never did follow up on the problem I had with the nested lock > elimination handling. :( ). Thanks for doing it. > Meanwhile this will need to be converted to a PR in any case. I hope to get the PR out later but we've got a team outing today... we haven't seen each other since months... :) Cheers, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/030911.html -Original Message- From: David Holmes Sent: Mittwoch, 9. September 2020 00:29 To: Reingruber, Richard ; Marty Thompson ; Daniel Daugherty ; serviceability-dev ; hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime ; Robbin Ehn ; Vladimir Kozlov Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, I suspect this one fell off the radar due to the extended review period. The actual review started last December (there was prior discussion IIRC) and only seemed to get partial reviews. I only looked at some parts. Robbin may have given things a deeper look, but seemed focused on the handshake aspects. Vladimir said he would do a full review but I can't find it. Eventually Martin and Goetz took over reviewing and everyone else dropped off. :( As this covers a number of areas it really does need "approval" from each area (and yes the hotspot wiki should reflect this). I will try to take another look while we await Serguei's return (and I never did follow up on the problem I had with the nested lock elimination handling. :( ). Meanwhile this will need to be converted to a PR in any case. Thanks, David On 9/09/2020 3:02 am, Reingruber, Richard wrote: > Hello Marty, > > Sure. I'd be happy if Serguei could review the change. > > Thanks, Richard. > > -Original Message- > From: Marty Thompson > Sent: Dienstag, 8. September 2020 18:55 > To: Reingruber, Richard ; Daniel Daugherty > ; serviceability-dev > ; hotspot-compiler-...@openjdk.java.net; > Hotspot dev runtime > Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in > the Presence of JVMTI Agents > > Hello Richard, > > It would be good if Serguei Spitsyn could review before this is pushed. > Serguei is out this week. Can you wait until Serguei is back in the office > the week of Sept 14? > > Regards, > > Marty > >> -Original Message- >> From: Reingruber, Richard >> Sent: Tuesday, September 8, 2020 9:45 AM >> To: Daniel Daugherty ; serviceability-dev >> ; hotspot-compiler- >> d...@openjdk.java.net; Hotspot dev runtime > d...@openjdk.java.net> >> Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance >> in the Presence of JVMTI Agents >> >> Hi Dan, >> >> I'd be very happy about a review from somebody on the Serviceability team. >> I have asked for reviews many times (kindly I hope). And the change is for >> review for more than a year now. >> >> According to [1] I'd think all requirements to push are met already. But >> maybe I missed something? >> >> After renaming of methods in SafepointMechanism the change needs to be >> rebased (already done). I'll publish a pull request as soon as possible. >> >> Thanks, Richard. >> >> [1] >> https://wiki.openjdk.java.net/display/HotSpot/Pushing+a+HotSpot+change >> >> -Original Message- >> From: Daniel D. Daugherty >> Sent: Dienstag, 8. September 2020 18:16 >> To: Reingruber, Richard ; serviceability-dev >> ; hotspot-compiler- >> d...@openjdk.java.net; Hotspot dev runtime > d...@openjdk.java.net> >> Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance >> in the P
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, I suspect this one fell off the radar due to the extended review period. The actual review started last December (there was prior discussion IIRC) and only seemed to get partial reviews. I only looked at some parts. Robbin may have given things a deeper look, but seemed focused on the handshake aspects. Vladimir said he would do a full review but I can't find it. Eventually Martin and Goetz took over reviewing and everyone else dropped off. :( As this covers a number of areas it really does need "approval" from each area (and yes the hotspot wiki should reflect this). I will try to take another look while we await Serguei's return (and I never did follow up on the problem I had with the nested lock elimination handling. :( ). Meanwhile this will need to be converted to a PR in any case. Thanks, David On 9/09/2020 3:02 am, Reingruber, Richard wrote: Hello Marty, Sure. I'd be happy if Serguei could review the change. Thanks, Richard. -Original Message- From: Marty Thompson Sent: Dienstag, 8. September 2020 18:55 To: Reingruber, Richard ; Daniel Daugherty ; serviceability-dev ; hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hello Richard, It would be good if Serguei Spitsyn could review before this is pushed. Serguei is out this week. Can you wait until Serguei is back in the office the week of Sept 14? Regards, Marty -Original Message- From: Reingruber, Richard Sent: Tuesday, September 8, 2020 9:45 AM To: Daniel Daugherty ; serviceability-dev ; hotspot-compiler- d...@openjdk.java.net; Hotspot dev runtime Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Dan, I'd be very happy about a review from somebody on the Serviceability team. I have asked for reviews many times (kindly I hope). And the change is for review for more than a year now. According to [1] I'd think all requirements to push are met already. But maybe I missed something? After renaming of methods in SafepointMechanism the change needs to be rebased (already done). I'll publish a pull request as soon as possible. Thanks, Richard. [1] https://wiki.openjdk.java.net/display/HotSpot/Pushing+a+HotSpot+change -Original Message- From: Daniel D. Daugherty Sent: Dienstag, 8. September 2020 18:16 To: Reingruber, Richard ; serviceability-dev ; hotspot-compiler- d...@openjdk.java.net; Hotspot dev runtime Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, I haven't seen a review from anyone on the Serviceability team and I think you should get a review from them since JVM/TI is involved. Perhaps I missed it... Dan On 9/7/20 10:09 AM, Reingruber, Richard wrote: Hi, I would like to close the review of this change. It has received a lot of helpful feedback during the process and 2 full Reviews. Thanks everybody! I'm planning to push it this week on Thursday as solution for JBS items: https://bugs.openjdk.java.net/browse/JDK-8227745 https://bugs.openjdk.java.net/browse/JDK-8233915 Version to be pushed: http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ Hope to get my GIT/Skara setup going until then... :) Thanks, Richard. -Original Message- From: hotspot-compiler-dev On Behalf Of Reingruber, Richard Sent: Mittwoch, 2. September 2020 23:27 To: Robbin Ehn ; serviceability-dev ; hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime Subject: [CAUTION] RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Robin, On 2020-09-02 15:48, Reingruber, Richard wrote: Hi Robbin, // taking the discussion back to the mailing lists > I still don't understand why you don't deoptimize the objects inside the > handshake/safepoint instead? So for handshakes using asynch handshake and allowing blocking inside would fix that. (future fix, I'm working on that now) Just to make it clear: I'm not fond of the extra suspension mechanism currently used for JDK-8227745 either. I want to get rid of it and I will work on it. Asynch handshakes (JDK-8238761) could be a replacement for it. At least I think they can be used to suspend the target thread. For safepoint, since we have suspended all threads, ~'safepointed them' with a JavaThread, you _could_ just execute the action directly (e.g. skipping VM_HeapWalkOperation safepoint) since they are suppose to be safely suspended until the destructor of EB, no? Yes, this should be possible. This would be an advanced change though. I would like EscapeBarriers to be a no-op and fall back to current implementation, if C2-EscapeAnalysis/Graal are disabled. So I suggest future work to instead just execute the safepoint with t
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hello Marty, Sure. I'd be happy if Serguei could review the change. Thanks, Richard. -Original Message- From: Marty Thompson Sent: Dienstag, 8. September 2020 18:55 To: Reingruber, Richard ; Daniel Daugherty ; serviceability-dev ; hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hello Richard, It would be good if Serguei Spitsyn could review before this is pushed. Serguei is out this week. Can you wait until Serguei is back in the office the week of Sept 14? Regards, Marty > -Original Message- > From: Reingruber, Richard > Sent: Tuesday, September 8, 2020 9:45 AM > To: Daniel Daugherty ; serviceability-dev > ; hotspot-compiler- > d...@openjdk.java.net; Hotspot dev runtime d...@openjdk.java.net> > Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance > in the Presence of JVMTI Agents > > Hi Dan, > > I'd be very happy about a review from somebody on the Serviceability team. > I have asked for reviews many times (kindly I hope). And the change is for > review for more than a year now. > > According to [1] I'd think all requirements to push are met already. But > maybe I missed something? > > After renaming of methods in SafepointMechanism the change needs to be > rebased (already done). I'll publish a pull request as soon as possible. > > Thanks, Richard. > > [1] > https://wiki.openjdk.java.net/display/HotSpot/Pushing+a+HotSpot+change > > -Original Message- > From: Daniel D. Daugherty > Sent: Dienstag, 8. September 2020 18:16 > To: Reingruber, Richard ; serviceability-dev > ; hotspot-compiler- > d...@openjdk.java.net; Hotspot dev runtime d...@openjdk.java.net> > Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance > in the Presence of JVMTI Agents > > Hi Richard, > > I haven't seen a review from anyone on the Serviceability team and I think > you should get a review from them since JVM/TI is involved. > Perhaps I missed it... > > Dan > > > On 9/7/20 10:09 AM, Reingruber, Richard wrote: > > Hi, > > > > I would like to close the review of this change. > > > > It has received a lot of helpful feedback during the process and 2 > > full Reviews. Thanks everybody! > > > > I'm planning to push it this week on Thursday as solution for JBS items: > > > > https://bugs.openjdk.java.net/browse/JDK-8227745 > > https://bugs.openjdk.java.net/browse/JDK-8233915 > > > > Version to be pushed: > > > > http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ > > > > Hope to get my GIT/Skara setup going until then... :) > > > > Thanks, Richard. > > > > -----Original Message- > > From: hotspot-compiler-dev > > On Behalf Of Reingruber, > > Richard > > Sent: Mittwoch, 2. September 2020 23:27 > > To: Robbin Ehn ; serviceability-dev > > ; > > hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime > > > > Subject: [CAUTION] RE: RFR(L) 8227745: Enable Escape Analysis for > > Better Performance in the Presence of JVMTI Agents > > > > Hi Robin, > > > >> On 2020-09-02 15:48, Reingruber, Richard wrote: > >>> Hi Robbin, > >>> > >>> // taking the discussion back to the mailing lists > >>> > >>> > I still don't understand why you don't deoptimize the objects inside > the > >>> > handshake/safepoint instead? > >> So for handshakes using asynch handshake and allowing blocking inside > >> would fix that. (future fix, I'm working on that now) > > Just to make it clear: I'm not fond of the extra suspension mechanism > > currently used for JDK-8227745 either. I want to get rid of it and I > > will work on it. Asynch handshakes (JDK-8238761) could be a > > replacement for it. At least I think they can be used to suspend the target > thread. > > > >> For safepoint, since we have suspended all threads, ~'safepointed them' > >> with a JavaThread, you _could_ just execute the action directly (e.g. > >> skipping VM_HeapWalkOperation safepoint) since they are suppose to be > >> safely suspended until the destructor of EB, no? > > Yes, this should be possible. This would be an advanced change though. > > I would like EscapeBarriers to be a no-op and fall back to current > > implementation, if C2-EscapeAnalysis/Graal are disabled. > > > >> So I suggest future work to instead just execute the safepoint with >
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hello Richard, It would be good if Serguei Spitsyn could review before this is pushed. Serguei is out this week. Can you wait until Serguei is back in the office the week of Sept 14? Regards, Marty > -Original Message- > From: Reingruber, Richard > Sent: Tuesday, September 8, 2020 9:45 AM > To: Daniel Daugherty ; serviceability-dev > ; hotspot-compiler- > d...@openjdk.java.net; Hotspot dev runtime d...@openjdk.java.net> > Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance > in the Presence of JVMTI Agents > > Hi Dan, > > I'd be very happy about a review from somebody on the Serviceability team. > I have asked for reviews many times (kindly I hope). And the change is for > review for more than a year now. > > According to [1] I'd think all requirements to push are met already. But > maybe I missed something? > > After renaming of methods in SafepointMechanism the change needs to be > rebased (already done). I'll publish a pull request as soon as possible. > > Thanks, Richard. > > [1] > https://wiki.openjdk.java.net/display/HotSpot/Pushing+a+HotSpot+change > > -Original Message- > From: Daniel D. Daugherty > Sent: Dienstag, 8. September 2020 18:16 > To: Reingruber, Richard ; serviceability-dev > ; hotspot-compiler- > d...@openjdk.java.net; Hotspot dev runtime d...@openjdk.java.net> > Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance > in the Presence of JVMTI Agents > > Hi Richard, > > I haven't seen a review from anyone on the Serviceability team and I think > you should get a review from them since JVM/TI is involved. > Perhaps I missed it... > > Dan > > > On 9/7/20 10:09 AM, Reingruber, Richard wrote: > > Hi, > > > > I would like to close the review of this change. > > > > It has received a lot of helpful feedback during the process and 2 > > full Reviews. Thanks everybody! > > > > I'm planning to push it this week on Thursday as solution for JBS items: > > > > https://bugs.openjdk.java.net/browse/JDK-8227745 > > https://bugs.openjdk.java.net/browse/JDK-8233915 > > > > Version to be pushed: > > > > http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ > > > > Hope to get my GIT/Skara setup going until then... :) > > > > Thanks, Richard. > > > > -Original Message----- > > From: hotspot-compiler-dev > > On Behalf Of Reingruber, > > Richard > > Sent: Mittwoch, 2. September 2020 23:27 > > To: Robbin Ehn ; serviceability-dev > > ; > > hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime > > > > Subject: [CAUTION] RE: RFR(L) 8227745: Enable Escape Analysis for > > Better Performance in the Presence of JVMTI Agents > > > > Hi Robin, > > > >> On 2020-09-02 15:48, Reingruber, Richard wrote: > >>> Hi Robbin, > >>> > >>> // taking the discussion back to the mailing lists > >>> > >>> > I still don't understand why you don't deoptimize the objects inside > the > >>> > handshake/safepoint instead? > >> So for handshakes using asynch handshake and allowing blocking inside > >> would fix that. (future fix, I'm working on that now) > > Just to make it clear: I'm not fond of the extra suspension mechanism > > currently used for JDK-8227745 either. I want to get rid of it and I > > will work on it. Asynch handshakes (JDK-8238761) could be a > > replacement for it. At least I think they can be used to suspend the target > thread. > > > >> For safepoint, since we have suspended all threads, ~'safepointed them' > >> with a JavaThread, you _could_ just execute the action directly (e.g. > >> skipping VM_HeapWalkOperation safepoint) since they are suppose to be > >> safely suspended until the destructor of EB, no? > > Yes, this should be possible. This would be an advanced change though. > > I would like EscapeBarriers to be a no-op and fall back to current > > implementation, if C2-EscapeAnalysis/Graal are disabled. > > > >> So I suggest future work to instead just execute the safepoint with > >> the requesting JT instead of having a this special safepoiting mechanism. > >> Since you are missing above functionality I see why you went this way. > >> If you need to push it, it's fine by me. > > We will work on further improvements. Top of the list would be > > eliminating the extra suspend mechanism. > > > > The implementation has matured for more than 12 mon
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Dan, I'd be very happy about a review from somebody on the Serviceability team. I have asked for reviews many times (kindly I hope). And the change is for review for more than a year now. According to [1] I'd think all requirements to push are met already. But maybe I missed something? After renaming of methods in SafepointMechanism the change needs to be rebased (already done). I'll publish a pull request as soon as possible. Thanks, Richard. [1] https://wiki.openjdk.java.net/display/HotSpot/Pushing+a+HotSpot+change -Original Message- From: Daniel D. Daugherty Sent: Dienstag, 8. September 2020 18:16 To: Reingruber, Richard ; serviceability-dev ; hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, I haven't seen a review from anyone on the Serviceability team and I think you should get a review from them since JVM/TI is involved. Perhaps I missed it... Dan On 9/7/20 10:09 AM, Reingruber, Richard wrote: > Hi, > > I would like to close the review of this change. > > It has received a lot of helpful feedback during the process and 2 full > Reviews. Thanks everybody! > > I'm planning to push it this week on Thursday as solution for JBS items: > > https://bugs.openjdk.java.net/browse/JDK-8227745 > https://bugs.openjdk.java.net/browse/JDK-8233915 > > Version to be pushed: > > http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ > > Hope to get my GIT/Skara setup going until then... :) > > Thanks, Richard. > > -Original Message- > From: hotspot-compiler-dev On > Behalf Of Reingruber, Richard > Sent: Mittwoch, 2. September 2020 23:27 > To: Robbin Ehn ; serviceability-dev > ; hotspot-compiler-...@openjdk.java.net; > Hotspot dev runtime > Subject: [CAUTION] RE: RFR(L) 8227745: Enable Escape Analysis for Better > Performance in the Presence of JVMTI Agents > > Hi Robin, > >> On 2020-09-02 15:48, Reingruber, Richard wrote: >>> Hi Robbin, >>> >>> // taking the discussion back to the mailing lists >>> >>> > I still don't understand why you don't deoptimize the objects inside >>> the >>> > handshake/safepoint instead? >> So for handshakes using asynch handshake and allowing blocking inside >> would fix that. (future fix, I'm working on that now) > Just to make it clear: I'm not fond of the extra suspension mechanism > currently > used for JDK-8227745 either. I want to get rid of it and I will work on it. > Asynch > handshakes (JDK-8238761) could be a replacement for it. At least I think they > can be used to suspend the target thread. > >> For safepoint, since we have suspended all threads, ~'safepointed them' >> with a JavaThread, you _could_ just execute the action directly (e.g. >> skipping VM_HeapWalkOperation safepoint) since they are suppose to be >> safely suspended until the destructor of EB, no? > Yes, this should be possible. This would be an advanced change though. I would > like EscapeBarriers to be a no-op and fall back to current implementation, if > C2-EscapeAnalysis/Graal are disabled. > >> So I suggest future work to instead just execute the safepoint with the >> requesting JT instead of having a this special safepoiting mechanism. >> Since you are missing above functionality I see why you went this way. >> If you need to push it, it's fine by me. > We will work on further improvements. Top of the list would > be eliminating the extra suspend mechanism. > > The implementation has matured for more than 12 months now [1]. It's been > tested > extensively at SAP over that time and passed also extended testing at Oracle > kindly conducted by Vladimir Kozlov. We've got two full Reviews and > incorporated > extensive feedback from a number of OpenJDK Reviewers (including you, > thanks!). Based on that I reckon we're good to push the change as enhancement > (JDK-8227745) and bug fix (JDK-8233915). > >> Thanks for explaining once again :) > Pleasure :) > > Thanks, Richard. > > [1] > http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-July/028729.html > > -Original Message- > From: Robbin Ehn > Sent: Mittwoch, 2. September 2020 16:54 > To: Reingruber, Richard ; serviceability-dev > ; hotspot-compiler-...@openjdk.java.net; > Hotspot dev runtime > Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in > the Presence of JVMTI Agents > > Hi Richard, > > On 2020-09-02 15:48, Reingruber, Richard wrote: >> Hi Robbin, >> >> // takin
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, I haven't seen a review from anyone on the Serviceability team and I think you should get a review from them since JVM/TI is involved. Perhaps I missed it... Dan On 9/7/20 10:09 AM, Reingruber, Richard wrote: Hi, I would like to close the review of this change. It has received a lot of helpful feedback during the process and 2 full Reviews. Thanks everybody! I'm planning to push it this week on Thursday as solution for JBS items: https://bugs.openjdk.java.net/browse/JDK-8227745 https://bugs.openjdk.java.net/browse/JDK-8233915 Version to be pushed: http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ Hope to get my GIT/Skara setup going until then... :) Thanks, Richard. -Original Message- From: hotspot-compiler-dev On Behalf Of Reingruber, Richard Sent: Mittwoch, 2. September 2020 23:27 To: Robbin Ehn ; serviceability-dev ; hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime Subject: [CAUTION] RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Robin, On 2020-09-02 15:48, Reingruber, Richard wrote: Hi Robbin, // taking the discussion back to the mailing lists > I still don't understand why you don't deoptimize the objects inside the > handshake/safepoint instead? So for handshakes using asynch handshake and allowing blocking inside would fix that. (future fix, I'm working on that now) Just to make it clear: I'm not fond of the extra suspension mechanism currently used for JDK-8227745 either. I want to get rid of it and I will work on it. Asynch handshakes (JDK-8238761) could be a replacement for it. At least I think they can be used to suspend the target thread. For safepoint, since we have suspended all threads, ~'safepointed them' with a JavaThread, you _could_ just execute the action directly (e.g. skipping VM_HeapWalkOperation safepoint) since they are suppose to be safely suspended until the destructor of EB, no? Yes, this should be possible. This would be an advanced change though. I would like EscapeBarriers to be a no-op and fall back to current implementation, if C2-EscapeAnalysis/Graal are disabled. So I suggest future work to instead just execute the safepoint with the requesting JT instead of having a this special safepoiting mechanism. Since you are missing above functionality I see why you went this way. If you need to push it, it's fine by me. We will work on further improvements. Top of the list would be eliminating the extra suspend mechanism. The implementation has matured for more than 12 months now [1]. It's been tested extensively at SAP over that time and passed also extended testing at Oracle kindly conducted by Vladimir Kozlov. We've got two full Reviews and incorporated extensive feedback from a number of OpenJDK Reviewers (including you, thanks!). Based on that I reckon we're good to push the change as enhancement (JDK-8227745) and bug fix (JDK-8233915). Thanks for explaining once again :) Pleasure :) Thanks, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-July/028729.html -Original Message- From: Robbin Ehn Sent: Mittwoch, 2. September 2020 16:54 To: Reingruber, Richard ; serviceability-dev ; hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, On 2020-09-02 15:48, Reingruber, Richard wrote: Hi Robbin, // taking the discussion back to the mailing lists > I still don't understand why you don't deoptimize the objects inside the > handshake/safepoint instead? So for handshakes using asynch handshake and allowing blocking inside would fix that. (future fix, I'm working on that now) For safepoint, since we have suspended all threads, ~'safepointed them' with a JavaThread, you _could_ just execute the action directly (e.g. skipping VM_HeapWalkOperation safepoint) since they are suppose to be safely suspended until the destructor of EB, no? So I suggest future work to instead just execute the safepoint with the requesting JT instead of having a this special safepoiting mechanism. Since you are missing above functionality I see why you went this way. If you need to push it, it's fine by me. Thanks for explaining once again :) /Robbin This is unfortunately not possible. Deoptimizing objects includes reallocating scalar replaced objects, i.e. calling Deoptimization::realloc_objects(). This cannot be done at a safepoint or handshake. 1. The vm thread is not allowed to allocate on the java heap See for instance assertions in ParallelScavengeHeap::mem_allocate() https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/4c73e045ce815d52abcdc99499266ccf2e6e9b4c/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp*L258__;Iw!!GqivPVa7Brio!K0f5chjtePI6MKBSBOo
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi, I would like to close the review of this change. It has received a lot of helpful feedback during the process and 2 full Reviews. Thanks everybody! I'm planning to push it this week on Thursday as solution for JBS items: https://bugs.openjdk.java.net/browse/JDK-8227745 https://bugs.openjdk.java.net/browse/JDK-8233915 Version to be pushed: http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ Hope to get my GIT/Skara setup going until then... :) Thanks, Richard. -Original Message- From: hotspot-compiler-dev On Behalf Of Reingruber, Richard Sent: Mittwoch, 2. September 2020 23:27 To: Robbin Ehn ; serviceability-dev ; hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime Subject: [CAUTION] RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Robin, > On 2020-09-02 15:48, Reingruber, Richard wrote: > > Hi Robbin, > > > > // taking the discussion back to the mailing lists > > > >> I still don't understand why you don't deoptimize the objects inside > > the > >> handshake/safepoint instead? > So for handshakes using asynch handshake and allowing blocking inside > would fix that. (future fix, I'm working on that now) Just to make it clear: I'm not fond of the extra suspension mechanism currently used for JDK-8227745 either. I want to get rid of it and I will work on it. Asynch handshakes (JDK-8238761) could be a replacement for it. At least I think they can be used to suspend the target thread. > For safepoint, since we have suspended all threads, ~'safepointed them' > with a JavaThread, you _could_ just execute the action directly (e.g. > skipping VM_HeapWalkOperation safepoint) since they are suppose to be > safely suspended until the destructor of EB, no? Yes, this should be possible. This would be an advanced change though. I would like EscapeBarriers to be a no-op and fall back to current implementation, if C2-EscapeAnalysis/Graal are disabled. > So I suggest future work to instead just execute the safepoint with the > requesting JT instead of having a this special safepoiting mechanism. > Since you are missing above functionality I see why you went this way. > If you need to push it, it's fine by me. We will work on further improvements. Top of the list would be eliminating the extra suspend mechanism. The implementation has matured for more than 12 months now [1]. It's been tested extensively at SAP over that time and passed also extended testing at Oracle kindly conducted by Vladimir Kozlov. We've got two full Reviews and incorporated extensive feedback from a number of OpenJDK Reviewers (including you, thanks!). Based on that I reckon we're good to push the change as enhancement (JDK-8227745) and bug fix (JDK-8233915). > Thanks for explaining once again :) Pleasure :) Thanks, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-July/028729.html -Original Message- From: Robbin Ehn Sent: Mittwoch, 2. September 2020 16:54 To: Reingruber, Richard ; serviceability-dev ; hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, On 2020-09-02 15:48, Reingruber, Richard wrote: > Hi Robbin, > > // taking the discussion back to the mailing lists > >> I still don't understand why you don't deoptimize the objects inside the >> handshake/safepoint instead? So for handshakes using asynch handshake and allowing blocking inside would fix that. (future fix, I'm working on that now) For safepoint, since we have suspended all threads, ~'safepointed them' with a JavaThread, you _could_ just execute the action directly (e.g. skipping VM_HeapWalkOperation safepoint) since they are suppose to be safely suspended until the destructor of EB, no? So I suggest future work to instead just execute the safepoint with the requesting JT instead of having a this special safepoiting mechanism. Since you are missing above functionality I see why you went this way. If you need to push it, it's fine by me. Thanks for explaining once again :) /Robbin > > This is unfortunately not possible. Deoptimizing objects includes reallocating > scalar replaced objects, i.e. calling Deoptimization::realloc_objects(). This > cannot be done at a safepoint or handshake. > > 1. The vm thread is not allowed to allocate on the java heap > See for instance assertions in ParallelScavengeHeap::mem_allocate() > > https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/4c73e045ce815d52abcdc99499266ccf2e6e9b4c/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp*L258__;Iw!!GqivPVa7Brio!K0f5chjtePI6MKBSBOoBKya9YZTJlVhsExQYMDO96v3Af_Klc_E4R26_d
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Robin, > On 2020-09-02 15:48, Reingruber, Richard wrote: > > Hi Robbin, > > > > // taking the discussion back to the mailing lists > > > >> I still don't understand why you don't deoptimize the objects inside > > the > >> handshake/safepoint instead? > So for handshakes using asynch handshake and allowing blocking inside > would fix that. (future fix, I'm working on that now) Just to make it clear: I'm not fond of the extra suspension mechanism currently used for JDK-8227745 either. I want to get rid of it and I will work on it. Asynch handshakes (JDK-8238761) could be a replacement for it. At least I think they can be used to suspend the target thread. > For safepoint, since we have suspended all threads, ~'safepointed them' > with a JavaThread, you _could_ just execute the action directly (e.g. > skipping VM_HeapWalkOperation safepoint) since they are suppose to be > safely suspended until the destructor of EB, no? Yes, this should be possible. This would be an advanced change though. I would like EscapeBarriers to be a no-op and fall back to current implementation, if C2-EscapeAnalysis/Graal are disabled. > So I suggest future work to instead just execute the safepoint with the > requesting JT instead of having a this special safepoiting mechanism. > Since you are missing above functionality I see why you went this way. > If you need to push it, it's fine by me. We will work on further improvements. Top of the list would be eliminating the extra suspend mechanism. The implementation has matured for more than 12 months now [1]. It's been tested extensively at SAP over that time and passed also extended testing at Oracle kindly conducted by Vladimir Kozlov. We've got two full Reviews and incorporated extensive feedback from a number of OpenJDK Reviewers (including you, thanks!). Based on that I reckon we're good to push the change as enhancement (JDK-8227745) and bug fix (JDK-8233915). > Thanks for explaining once again :) Pleasure :) Thanks, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-July/028729.html -Original Message- From: Robbin Ehn Sent: Mittwoch, 2. September 2020 16:54 To: Reingruber, Richard ; serviceability-dev ; hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, On 2020-09-02 15:48, Reingruber, Richard wrote: > Hi Robbin, > > // taking the discussion back to the mailing lists > >> I still don't understand why you don't deoptimize the objects inside the >> handshake/safepoint instead? So for handshakes using asynch handshake and allowing blocking inside would fix that. (future fix, I'm working on that now) For safepoint, since we have suspended all threads, ~'safepointed them' with a JavaThread, you _could_ just execute the action directly (e.g. skipping VM_HeapWalkOperation safepoint) since they are suppose to be safely suspended until the destructor of EB, no? So I suggest future work to instead just execute the safepoint with the requesting JT instead of having a this special safepoiting mechanism. Since you are missing above functionality I see why you went this way. If you need to push it, it's fine by me. Thanks for explaining once again :) /Robbin > > This is unfortunately not possible. Deoptimizing objects includes reallocating > scalar replaced objects, i.e. calling Deoptimization::realloc_objects(). This > cannot be done at a safepoint or handshake. > > 1. The vm thread is not allowed to allocate on the java heap > See for instance assertions in ParallelScavengeHeap::mem_allocate() > > https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/4c73e045ce815d52abcdc99499266ccf2e6e9b4c/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp*L258__;Iw!!GqivPVa7Brio!K0f5chjtePI6MKBSBOoBKya9YZTJlVhsExQYMDO96v3Af_Klc_E4R26_dSyowotF$ > > This is not easy to change, I suppose, because it will be difficult to gc > if > necessary. > > 2. Using a direct handshake would not work either. The problem there is again > gc. Let J be the JavaThread that is executing the direct handshake. The vm > would deadlock if the vm thread waits for J to execute the closure of a > handshake-all and J waits for the vm thread to execute a gc vm operation. > Patricio Chilano made me aware of this: > https://bugs.openjdk.java.net/browse/JDK-8230594 > > Cheers, Richard. > > -----Original Message----- > From: Robbin Ehn > Sent: Mittwoch, 2. September 2020 13:56 > To: Reingruber, Richard > Cc: Lindenmaier, Goetz ; Vladimir Kozlov > ; David Holmes > Subject: Re: RFR(L) 8227745: En
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, On 2020-09-02 15:48, Reingruber, Richard wrote: Hi Robbin, // taking the discussion back to the mailing lists > I still don't understand why you don't deoptimize the objects inside the > handshake/safepoint instead? So for handshakes using asynch handshake and allowing blocking inside would fix that. (future fix, I'm working on that now) For safepoint, since we have suspended all threads, ~'safepointed them' with a JavaThread, you _could_ just execute the action directly (e.g. skipping VM_HeapWalkOperation safepoint) since they are suppose to be safely suspended until the destructor of EB, no? So I suggest future work to instead just execute the safepoint with the requesting JT instead of having a this special safepoiting mechanism. Since you are missing above functionality I see why you went this way. If you need to push it, it's fine by me. Thanks for explaining once again :) /Robbin This is unfortunately not possible. Deoptimizing objects includes reallocating scalar replaced objects, i.e. calling Deoptimization::realloc_objects(). This cannot be done at a safepoint or handshake. 1. The vm thread is not allowed to allocate on the java heap See for instance assertions in ParallelScavengeHeap::mem_allocate() https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/4c73e045ce815d52abcdc99499266ccf2e6e9b4c/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp*L258__;Iw!!GqivPVa7Brio!K0f5chjtePI6MKBSBOoBKya9YZTJlVhsExQYMDO96v3Af_Klc_E4R26_dSyowotF$ This is not easy to change, I suppose, because it will be difficult to gc if necessary. 2. Using a direct handshake would not work either. The problem there is again gc. Let J be the JavaThread that is executing the direct handshake. The vm would deadlock if the vm thread waits for J to execute the closure of a handshake-all and J waits for the vm thread to execute a gc vm operation. Patricio Chilano made me aware of this: https://bugs.openjdk.java.net/browse/JDK-8230594 Cheers, Richard. -Original Message- From: Robbin Ehn Sent: Mittwoch, 2. September 2020 13:56 To: Reingruber, Richard Cc: Lindenmaier, Goetz ; Vladimir Kozlov ; David Holmes Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi, I still don't understand why you don't deoptimize the objects inside the handshake/safepoint instead? E.g. JvmtiEnv::GetOwnedMonitorInfo you only should need the execute the code from: eb.deoptimize_objects(MaxJavaStackTraceDepth)) before looping over the stack, so: void GetOwnedMonitorInfoClosure::do_thread(Thread *target) { assert(target->is_Java_thread(), "just checking"); JavaThread *jt = (JavaThread *)target; if (!jt->is_exiting() && (jt->threadObj() != NULL)) { +if (EscapeBarrier::deoptimize_objects(jt, MaxJavaStackTraceDepth)) { _result = ((JvmtiEnvBase*)_env)->get_owned_monitors(_calling_thread, jt, _owned_monitors_list); } else { _result = JVMTI_ERROR_OUT_OF_MEMORY; } } } Why try 'suspend' the thread first? When we de-optimize all threads why not just in the following safepoint? E.g. VM_HeapWalkOperation::doit() { + EscapeBarrier::deoptimize_objects_all_threads(); ... } Thanks, Robbin
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Robbin, // taking the discussion back to the mailing lists > I still don't understand why you don't deoptimize the objects inside the > handshake/safepoint instead? This is unfortunately not possible. Deoptimizing objects includes reallocating scalar replaced objects, i.e. calling Deoptimization::realloc_objects(). This cannot be done at a safepoint or handshake. 1. The vm thread is not allowed to allocate on the java heap See for instance assertions in ParallelScavengeHeap::mem_allocate() https://github.com/openjdk/jdk/blob/4c73e045ce815d52abcdc99499266ccf2e6e9b4c/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp#L258 This is not easy to change, I suppose, because it will be difficult to gc if necessary. 2. Using a direct handshake would not work either. The problem there is again gc. Let J be the JavaThread that is executing the direct handshake. The vm would deadlock if the vm thread waits for J to execute the closure of a handshake-all and J waits for the vm thread to execute a gc vm operation. Patricio Chilano made me aware of this: https://bugs.openjdk.java.net/browse/JDK-8230594 Cheers, Richard. -Original Message- From: Robbin Ehn Sent: Mittwoch, 2. September 2020 13:56 To: Reingruber, Richard Cc: Lindenmaier, Goetz ; Vladimir Kozlov ; David Holmes Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi, I still don't understand why you don't deoptimize the objects inside the handshake/safepoint instead? E.g. JvmtiEnv::GetOwnedMonitorInfo you only should need the execute the code from: eb.deoptimize_objects(MaxJavaStackTraceDepth)) before looping over the stack, so: void GetOwnedMonitorInfoClosure::do_thread(Thread *target) { assert(target->is_Java_thread(), "just checking"); JavaThread *jt = (JavaThread *)target; if (!jt->is_exiting() && (jt->threadObj() != NULL)) { +if (EscapeBarrier::deoptimize_objects(jt, MaxJavaStackTraceDepth)) { _result = ((JvmtiEnvBase*)_env)->get_owned_monitors(_calling_thread, jt, _owned_monitors_list); } else { _result = JVMTI_ERROR_OUT_OF_MEMORY; } } } Why try 'suspend' the thread first? When we de-optimize all threads why not just in the following safepoint? E.g. VM_HeapWalkOperation::doit() { + EscapeBarrier::deoptimize_objects_all_threads(); ... } Thanks, Robbin
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Thanks a lot! Richard. -Original Message- From: Lindenmaier, Goetz Sent: Freitag, 28. August 2020 08:38 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, Thanks for the new webrev. The small improvements are fine, too. Reviewed from my side. Best regards, Goetz. > -Original Message- > From: Reingruber, Richard > Sent: Thursday, August 27, 2020 10:33 PM > To: Lindenmaier, Goetz ; serviceability- > d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot- > runtime-...@openjdk.java.net > Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance > in the Presence of JVMTI Agents > > Hi Goetz, > > > I read through your change again. It looks good to me now. > > The new naming and additional comments make it > > easier to read I think, thank you. > > Thanks for all your input! > > > One small thing: > > deoptimization.cpp, l. 1503 > > You don't really need the brackets. Two lines below you don't use them > either. > > (No webrev needed) > > Thanks for providing the correct line off list. Fixed! > > I prepared a new webrev, because I had to rebase after JDK-8249293 [1] and > because I wanted to make use of JDK-8251384 [2] > > Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ > Delta: http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8.inc/ > > The delta looks bigger than it is. Most of it is re-indentation of > VM_GetOrSetLocal::deoptimize_objects(). You can see this if you look at > > http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8.inc/src/hotsp > ot/share/prims/jvmtiImpl.cpp.udiff.html > > which does not include the whitespace change. > > Hope you are still ok with webrev.8. The changes are marginal. I've > commented > each below. > > Thanks, Richard. > > --- Details below --- > > src/hotspot/share/prims/jvmtiImpl.cpp > > @@ -425,11 +425,11 @@ >, _depth(depth) >, _index(index) >, _type(type) >, _jvf(NULL) >, _set(false) > - , _eb(NULL, NULL, false) // no references escape > + , _eb(NULL, NULL, type == T_OBJECT) >, _result(JVMTI_ERROR_NONE) > > Currently 'type' is never equal to T_OBJECT at this location, still I think it > is better to check. The compiler will replace the compare with false. > > @@ -630,11 +630,11 @@ > } > > // Revert optimizations based on escape analysis if this is an access to a > local object > bool VM_GetOrSetLocal::deoptimize_objects(javaVFrame* jvf) { > #if COMPILER2_OR_JVMCI > - if (NOT_JVMCI(DoEscapeAnalysis &&) _type == T_OBJECT) { > + assert(_type == T_OBJECT, "EscapeBarrier should not be active if _type != > T_OBJECT"); > > I removed the if from VM_GetOrSetLocal::deoptimize_objects(), because > now it > only gets called if the VM_GetOrSetLocal instance has an active > EscapeBarrier > which will be the case iff the local type is T_OBJECT and if either C2 escape > analysis is enabled or Graal is used. > > src/hotspot/share/runtime/deoptimization.cpp > > You suggested to remove the braces. Done. > > src/hotspot/share/runtime/deoptimization.hpp > > Must provide definition of EscapeBarrier::barrier_active() for new call site > in > VM_GetOrSetLocal::doit_prologue() if building with COMPILER2_OR_JVMCI > not > defined. > > test/hotspot/jtreg/serviceability/jvmti/Heap/IterateHeapWithEscapeAnalysis > Enabled.java > > Make use of [2] and pass test with minimal vm. > > [1] https://bugs.openjdk.java.net/browse/JDK-8249293 > [2] https://bugs.openjdk.java.net/browse/JDK-8251384 > > -Original Message- > From: Lindenmaier, Goetz > Sent: Samstag, 22. August 2020 07:46 > To: Reingruber, Richard ; serviceability- > d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot- > runtime-...@openjdk.java.net > Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance > in the Presence of JVMTI Agents > > Hi Richard, > > I read through your change again. It looks good to me now. > The new naming and additional comments make it > easier to read I think, thank you. > > One small thing: > deoptimization.cpp, l. 1503 > You don't really need the brackets. Two lines below you don't use them > either. > (No webrev needed) > > Best regards, > Goetz.
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, Thanks for the new webrev. The small improvements are fine, too. Reviewed from my side. Best regards, Goetz. > -Original Message- > From: Reingruber, Richard > Sent: Thursday, August 27, 2020 10:33 PM > To: Lindenmaier, Goetz ; serviceability- > d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot- > runtime-...@openjdk.java.net > Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance > in the Presence of JVMTI Agents > > Hi Goetz, > > > I read through your change again. It looks good to me now. > > The new naming and additional comments make it > > easier to read I think, thank you. > > Thanks for all your input! > > > One small thing: > > deoptimization.cpp, l. 1503 > > You don't really need the brackets. Two lines below you don't use them > either. > > (No webrev needed) > > Thanks for providing the correct line off list. Fixed! > > I prepared a new webrev, because I had to rebase after JDK-8249293 [1] and > because I wanted to make use of JDK-8251384 [2] > > Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ > Delta: http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8.inc/ > > The delta looks bigger than it is. Most of it is re-indentation of > VM_GetOrSetLocal::deoptimize_objects(). You can see this if you look at > > http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8.inc/src/hotsp > ot/share/prims/jvmtiImpl.cpp.udiff.html > > which does not include the whitespace change. > > Hope you are still ok with webrev.8. The changes are marginal. I've > commented > each below. > > Thanks, Richard. > > --- Details below --- > > src/hotspot/share/prims/jvmtiImpl.cpp > > @@ -425,11 +425,11 @@ >, _depth(depth) >, _index(index) >, _type(type) >, _jvf(NULL) >, _set(false) > - , _eb(NULL, NULL, false) // no references escape > + , _eb(NULL, NULL, type == T_OBJECT) >, _result(JVMTI_ERROR_NONE) > > Currently 'type' is never equal to T_OBJECT at this location, still I think it > is better to check. The compiler will replace the compare with false. > > @@ -630,11 +630,11 @@ > } > > // Revert optimizations based on escape analysis if this is an access to a > local object > bool VM_GetOrSetLocal::deoptimize_objects(javaVFrame* jvf) { > #if COMPILER2_OR_JVMCI > - if (NOT_JVMCI(DoEscapeAnalysis &&) _type == T_OBJECT) { > + assert(_type == T_OBJECT, "EscapeBarrier should not be active if _type != > T_OBJECT"); > > I removed the if from VM_GetOrSetLocal::deoptimize_objects(), because > now it > only gets called if the VM_GetOrSetLocal instance has an active > EscapeBarrier > which will be the case iff the local type is T_OBJECT and if either C2 escape > analysis is enabled or Graal is used. > > src/hotspot/share/runtime/deoptimization.cpp > > You suggested to remove the braces. Done. > > src/hotspot/share/runtime/deoptimization.hpp > > Must provide definition of EscapeBarrier::barrier_active() for new call site > in > VM_GetOrSetLocal::doit_prologue() if building with COMPILER2_OR_JVMCI > not > defined. > > test/hotspot/jtreg/serviceability/jvmti/Heap/IterateHeapWithEscapeAnalysis > Enabled.java > > Make use of [2] and pass test with minimal vm. > > [1] https://bugs.openjdk.java.net/browse/JDK-8249293 > [2] https://bugs.openjdk.java.net/browse/JDK-8251384 > > -Original Message- > From: Lindenmaier, Goetz > Sent: Samstag, 22. August 2020 07:46 > To: Reingruber, Richard ; serviceability- > d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot- > runtime-...@openjdk.java.net > Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance > in the Presence of JVMTI Agents > > Hi Richard, > > I read through your change again. It looks good to me now. > The new naming and additional comments make it > easier to read I think, thank you. > > One small thing: > deoptimization.cpp, l. 1503 > You don't really need the brackets. Two lines below you don't use them > either. > (No webrev needed) > > Best regards, > Goetz.
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Goetz, > I read through your change again. It looks good to me now. > The new naming and additional comments make it > easier to read I think, thank you. Thanks for all your input! > One small thing: > deoptimization.cpp, l. 1503 > You don't really need the brackets. Two lines below you don't use them either. > (No webrev needed) Thanks for providing the correct line off list. Fixed! I prepared a new webrev, because I had to rebase after JDK-8249293 [1] and because I wanted to make use of JDK-8251384 [2] Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ Delta: http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8.inc/ The delta looks bigger than it is. Most of it is re-indentation of VM_GetOrSetLocal::deoptimize_objects(). You can see this if you look at http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8.inc/src/hotspot/share/prims/jvmtiImpl.cpp.udiff.html which does not include the whitespace change. Hope you are still ok with webrev.8. The changes are marginal. I've commented each below. Thanks, Richard. --- Details below --- src/hotspot/share/prims/jvmtiImpl.cpp @@ -425,11 +425,11 @@ , _depth(depth) , _index(index) , _type(type) , _jvf(NULL) , _set(false) - , _eb(NULL, NULL, false) // no references escape + , _eb(NULL, NULL, type == T_OBJECT) , _result(JVMTI_ERROR_NONE) Currently 'type' is never equal to T_OBJECT at this location, still I think it is better to check. The compiler will replace the compare with false. @@ -630,11 +630,11 @@ } // Revert optimizations based on escape analysis if this is an access to a local object bool VM_GetOrSetLocal::deoptimize_objects(javaVFrame* jvf) { #if COMPILER2_OR_JVMCI - if (NOT_JVMCI(DoEscapeAnalysis &&) _type == T_OBJECT) { + assert(_type == T_OBJECT, "EscapeBarrier should not be active if _type != T_OBJECT"); I removed the if from VM_GetOrSetLocal::deoptimize_objects(), because now it only gets called if the VM_GetOrSetLocal instance has an active EscapeBarrier which will be the case iff the local type is T_OBJECT and if either C2 escape analysis is enabled or Graal is used. src/hotspot/share/runtime/deoptimization.cpp You suggested to remove the braces. Done. src/hotspot/share/runtime/deoptimization.hpp Must provide definition of EscapeBarrier::barrier_active() for new call site in VM_GetOrSetLocal::doit_prologue() if building with COMPILER2_OR_JVMCI not defined. test/hotspot/jtreg/serviceability/jvmti/Heap/IterateHeapWithEscapeAnalysisEnabled.java Make use of [2] and pass test with minimal vm. [1] https://bugs.openjdk.java.net/browse/JDK-8249293 [2] https://bugs.openjdk.java.net/browse/JDK-8251384 -Original Message- From: Lindenmaier, Goetz Sent: Samstag, 22. August 2020 07:46 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, I read through your change again. It looks good to me now. The new naming and additional comments make it easier to read I think, thank you. One small thing: deoptimization.cpp, l. 1503 You don't really need the brackets. Two lines below you don't use them either. (No webrev needed) Best regards, Goetz.
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, I read through your change again. It looks good to me now. The new naming and additional comments make it easier to read I think, thank you. One small thing: deoptimization.cpp, l. 1503 You don't really need the brackets. Two lines below you don't use them either. (No webrev needed) Best regards, Goetz. -Original Message- From: Reingruber, Richard Sent: Dienstag, 18. August 2020 10:44 To: Lindenmaier, Goetz ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Goetz, I have collected the changes based on your feedback in a new webrev: Webrev.7: http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.7/ Delta:http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.7.inc/ Most of the changes are renamings, commenting, and reformatting. Besides that ... - I converted the native agent of the test IterateHeapWithEscapeAnalysisEnabled from C to C++, because this seems to be preferred by serviceability developers. I also re-indented the file, but excluded this from the delta webrev. - I had to adapt test/jdk/com/sun/jdi/EATests.java to the fact that background compilation (-Xbatch) cannot be reliably disabled for JVMCI compilers. E.g. the compile broker will compile in the background if JVMCI is not yet fully initialized. Therefore it is possible that test cases are executed before the main test method is compiled on the highest level and then the test case fails. The higher the system load the higher the probability for this to happen. In webrev.7 I skip the compilation level check if the vm is configured to use the JVMCI compiler. I also answered you inline below. Thanks, Richard. -Original Message- From: Lindenmaier, Goetz Sent: Donnerstag, 23. Juli 2020 16:20 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, Thanks for your two further explanations in the other thread. That made the points clear to me. > > I was not that happy with the names saying not_global_escape > > and similar. I now agreed you have to use the terms of the escape > > analysis (NoEscape ArgEscape= throughout the runtime code. I'm still not > > happy with > > the 'not' in the term, I always try to expand the name to some > > sentence with a negated verb, but it makes no sense. > > For example, "has_not_global_escape_in_scope" expands to > > "Hasn't a global escape in its scope." in my thinking, which makes > > no sense. You probably mean > > "Has not-global escape in its scope." or "Has {ArgEscape|NoEscape} > > in its scope." > > > C2 is using the word "non" in this context, e.g., here > > alloc->is_non_escaping. > > There is also ConnectionGraph::not_global_escape() That talks about a single node that represents a single Object. An object has a single state wrt. ea. You use the term for safepoint which tracks a set of objects. Here, has_not_global_excape can mean 1. None of the several objects does escape globaly. 2. There is at least one object that escapes globaly. > > non obviously negates the adjective 'global', > > non-global or nonglobal even is a English term I find in the > > net. > > So what about "has_non_global_escape_in_scope?" > > And what about has_ea_local_in_scope? That's good. Please document somewhere that Ea_local == ArgEscape | NoEscape. That's what it is, right? > > Does jvmti specify that the same limits are used ...? > > ok on your side. > > I don't know and didn't find anything in a quick search. Ok, not your business. > > > jvmtiEnvBase.cpp ok > > jvmtiImpl.h|cpp ok > > jvmtiTagMap.cpp ok > > whitebox.cpp ok > > > deoptimization.cpp > > > line 177: Please break line > > line 246, 281: Please break line > > 1578, 1583, 1589, 1632, 1649, 1651 Break line > > > 1651: You use 'non'-terms, too: non-escaping :) > > I know :) At least here it is wrong I'd say. "...has to be a not escaping > obj..." > sounds better > (hopefully not only to my german ears). I thought the term non-escpaing makes it quite clear. I just wanted to point out that using non above would be similar to the wording here. > > IterateHeapWithEscapeAnalysisEnabled.java > > > line 415: > > msg("wait until target thread has set testMethod_result"); > > while (testMethod_resul
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Goetz, I have collected the changes based on your feedback in a new webrev: Webrev.7: http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.7/ Delta:http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.7.inc/ Most of the changes are renamings, commenting, and reformatting. Besides that ... - I converted the native agent of the test IterateHeapWithEscapeAnalysisEnabled from C to C++, because this seems to be preferred by serviceability developers. I also re-indented the file, but excluded this from the delta webrev. - I had to adapt test/jdk/com/sun/jdi/EATests.java to the fact that background compilation (-Xbatch) cannot be reliably disabled for JVMCI compilers. E.g. the compile broker will compile in the background if JVMCI is not yet fully initialized. Therefore it is possible that test cases are executed before the main test method is compiled on the highest level and then the test case fails. The higher the system load the higher the probability for this to happen. In webrev.7 I skip the compilation level check if the vm is configured to use the JVMCI compiler. I also answered you inline below. Thanks, Richard. -Original Message- From: Lindenmaier, Goetz Sent: Donnerstag, 23. Juli 2020 16:20 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, Thanks for your two further explanations in the other thread. That made the points clear to me. > > I was not that happy with the names saying not_global_escape > > and similar. I now agreed you have to use the terms of the escape > > analysis (NoEscape ArgEscape= throughout the runtime code. I'm still not > > happy with > > the 'not' in the term, I always try to expand the name to some > > sentence with a negated verb, but it makes no sense. > > For example, "has_not_global_escape_in_scope" expands to > > "Hasn't a global escape in its scope." in my thinking, which makes > > no sense. You probably mean > > "Has not-global escape in its scope." or "Has {ArgEscape|NoEscape} > > in its scope." > > > C2 is using the word "non" in this context, e.g., here > > alloc->is_non_escaping. > > There is also ConnectionGraph::not_global_escape() That talks about a single node that represents a single Object. An object has a single state wrt. ea. You use the term for safepoint which tracks a set of objects. Here, has_not_global_excape can mean 1. None of the several objects does escape globaly. 2. There is at least one object that escapes globaly. > > non obviously negates the adjective 'global', > > non-global or nonglobal even is a English term I find in the > > net. > > So what about "has_non_global_escape_in_scope?" > > And what about has_ea_local_in_scope? That's good. Please document somewhere that Ea_local == ArgEscape | NoEscape. That's what it is, right? > > Does jvmti specify that the same limits are used ...? > > ok on your side. > > I don't know and didn't find anything in a quick search. Ok, not your business. > > > jvmtiEnvBase.cpp ok > > jvmtiImpl.h|cpp ok > > jvmtiTagMap.cpp ok > > whitebox.cpp ok > > > deoptimization.cpp > > > line 177: Please break line > > line 246, 281: Please break line > > 1578, 1583, 1589, 1632, 1649, 1651 Break line > > > 1651: You use 'non'-terms, too: non-escaping :) > > I know :) At least here it is wrong I'd say. "...has to be a not escaping > obj..." > sounds better > (hopefully not only to my german ears). I thought the term non-escpaing makes it quite clear. I just wanted to point out that using non above would be similar to the wording here. > > IterateHeapWithEscapeAnalysisEnabled.java > > > line 415: > > msg("wait until target thread has set testMethod_result"); > > while (testMethod_result == 0) { > > Thread.sleep(50); > > } > > Might the test run into timeouts at this place? > > The field is volatile, i.e. it will be reloaded > > in each iteration. But will dontinline_testMethod > > write it back to main memory in time? > > You mean, the test could hang in that loop for a couple of minutes? I don't > think so. There are cache coherence protocols in place which will invalidate > stale data very timely. Ok, anyways, it would only be a hanging test. > > Ok. I've removed quite a lot of the occurrances. > > > Also, I like full sentences in comments. > > Especially for me as
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, Thanks for your two further explanations in the other thread. That made the points clear to me. > > I was not that happy with the names saying not_global_escape > > and similar. I now agreed you have to use the terms of the escape > > analysis (NoEscape ArgEscape= throughout the runtime code. I'm still not > > happy with > > the 'not' in the term, I always try to expand the name to some > > sentence with a negated verb, but it makes no sense. > > For example, "has_not_global_escape_in_scope" expands to > > "Hasn't a global escape in its scope." in my thinking, which makes > > no sense. You probably mean > > "Has not-global escape in its scope." or "Has {ArgEscape|NoEscape} > > in its scope." > > > C2 is using the word "non" in this context, e.g., here > > alloc->is_non_escaping. > > There is also ConnectionGraph::not_global_escape() That talks about a single node that represents a single Object. An object has a single state wrt. ea. You use the term for safepoint which tracks a set of objects. Here, has_not_global_excape can mean 1. None of the several objects does escape globaly. 2. There is at least one object that escapes globaly. > > non obviously negates the adjective 'global', > > non-global or nonglobal even is a English term I find in the > > net. > > So what about "has_non_global_escape_in_scope?" > > And what about has_ea_local_in_scope? That's good. Please document somewhere that Ea_local == ArgEscape | NoEscape. That's what it is, right? > > Does jvmti specify that the same limits are used ...? > > ok on your side. > > I don't know and didn't find anything in a quick search. Ok, not your business. > > > jvmtiEnvBase.cpp ok > > jvmtiImpl.h|cpp ok > > jvmtiTagMap.cpp ok > > whitebox.cpp ok > > > deoptimization.cpp > > > line 177: Please break line > > line 246, 281: Please break line > > 1578, 1583, 1589, 1632, 1649, 1651 Break line > > > 1651: You use 'non'-terms, too: non-escaping :) > > I know :) At least here it is wrong I'd say. "...has to be a not escaping > obj..." > sounds better > (hopefully not only to my german ears). I thought the term non-escpaing makes it quite clear. I just wanted to point out that using non above would be similar to the wording here. > > IterateHeapWithEscapeAnalysisEnabled.java > > > line 415: > > msg("wait until target thread has set testMethod_result"); > > while (testMethod_result == 0) { > > Thread.sleep(50); > > } > > Might the test run into timeouts at this place? > > The field is volatile, i.e. it will be reloaded > > in each iteration. But will dontinline_testMethod > > write it back to main memory in time? > > You mean, the test could hang in that loop for a couple of minutes? I don't > think so. There are cache coherence protocols in place which will invalidate > stale data very timely. Ok, anyways, it would only be a hanging test. > > Ok. I've removed quite a lot of the occurrances. > > > Also, I like full sentences in comments. > > Especially for me as foreign speaker, this makes > > things much more clear. I.e., I try to make it > > a real sentence with articles, capitalized and a > > dot at the end if there is a subject and a verb > > in first place. > > E.g., jvmtiEnvBase.cpp:1327 > > Are you referring to the following? > (from > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.6/src/hots > pot/share/prims/jvmtiEnvBase.cpp.frames.html) > > 1326 > 1327 // If the frame is a compiled one, need to deoptimize it. > 1328 if (vf->is_compiled_frame()) { > > This line 1327 is preexisting. Sorry, wrong line number again. I think I meant 1333 // eagerly reallocate scalar replaced objects. But I must admit, the subject is missing. It's one of these imperative sentences where the subject is left out, which are used throughout documentation. Bad example, but still a correct sentence, so qualifies for punctuation? Best regards, Goetz.
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Goetz, > Thanks for the quick reply. Yes, this time it didn't take that long... [... snip ...] > > > > > I understand you annotate at safepoints where the escape analysis > > > > > finds out that an object is "better" than global escape. > > > > > This are the cases where the analysis identifies optimization > > > > > opportunities. These annotations are then used to deoptimize > > > > > frames and the objects referenced by them. > > > > > Doesn't this overestimate the optimized > > > > > objects? E.g., eliminate_alloc_node has many cases where it bails > > > > > out. > > > > > > > > Yes, the implementation is conservative, but it is comparatively simple > > and > > > > the additional debug > > > > info is just 2 flags per safepoint. > > > Thanks. It also helped that you explained to me offline that > > > there are more optimizations than only lock elimination and scalar > > > replacement done based on the ea information. > > > The ea refines the IR graph with allows follow up optimizations > > > which can not easily be tracked back to the escaping objects or > > > the call sites where they do not escape. > > > Thus, if there are non-global escaping objects, you have to > > > deoptimize the frame. > > > Did I repeat that correctly? > > > > Mostly, but there are also cases where deoptimization is required if and > > only > > if ea-local objects > > are passed as arguments. This is the case when values are not read directly > > from a frame, but from a callee frame. > Hmm, don't get this completely, but ok. Let C be a callee frame of B which is a callee of A. If you use JVMTI to read an object reference from a local variable of C then the implementation of JDK-8227745 deoptimizes A if it passes any ea-local as argument, because the reference could be ea-local in A and there might be optimizations that are invalid after the escape state change. > > > > Accesses to instance > > > > members or array elements can be optimized as well. > > > You mean the compiler can/will ignore volatile or memory ordering > > > requirements for non-escaping objects? Sounds reasonable to do. > > > > Yes, for instance. Also without volatile modifiers it will eliminate > > accesses. > > Here is an example: > > Method A has a NoEscape allocation O that is not scalar replaced. A calls > > Method B, which is not > > inlined. When you use your debugger to break in B, then modify a field of O, > > then this modification > > would have no effect without deoptimization, because the jit assumes that B > > cannot modify O without > > a reference to it. > Yes, A can keep O in a register, while the JVMTI thread would write to > the location in the stack where the local is held (if it was written back). Not quite. It is the value of the field of O that is in a register not the reference to O itself. The agent changes the field's value in the /java heap/ (remember: O is _not_ scalar replaced), but the fields value is not reloaded after return from B. > > > > > Syncronization: looks good. I think others had a look at this before. > > > > > > > > > > EscapeBarrier::deoptimize_objects_internal() > > > > > The method name is misleading, it is not used by > > > > > deoptimize_objects(). > > > > > Also, method with the same name is in Deopitmization. > > > > > Proposal: deoptimize_objects_thread() ? > > > > > > > > Sorry, but I don't see, why it would be misleading. > > > > What would be the meaning of 'deoptimize_objects_thread'? I don't > > > > understand that name. > > > 1. I have no idea why it's called "_internal". Because it is private? > > >By the name, I would expect that EscapeBarrier::deoptimize_objects() > > >calls it for some internal tasks. But it does not. > > > > Well, I'd say it is pretty internal, what's happening in that method. So > > IMHO > > the suffix _internal > > is a match. > > > > > 2. My proposal: deoptimize_objects_all_threads() iterates all threads > > > and calls deoptimize_objects(_one)_thread(thread) for each of these. > > > That's how I would have named it. > > > But no bike shedding, if you don't see what I mean it's not obvious. > > Ok. We could have a quick call, too, if you like. > Ok, I think I have understood the remai
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
cough > syrup, and then embarking on an adventure while wandering around > neighborhoods or parks all night. This is usually done while listening to > Punk rock music from a portable jambox. > ;) > Don’t do it! 😊 OMG! How come you know?! ;) > EATestsJVMTI.java > I think you can just copy this test description into the other > test. You can have two @test comments, they will be treated > as separate tests. The @requires will be evaluated accordingly. > For an example see > test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NullPointerExceptionTest.java > which has two different compile setups for the test class (-g). Done. > so, that's it for reading code ... > Some general remarks, maybe a bit picky ...: > I think you could use less commas ',' in comments. > As I understand, you need a comma if the relative > sentence is at the beginning, but not if it is at > the end: > If Corona is over, I go to the office. > but > I go to the office if Corona is over. That seem's to be correct except "If Corona is over" isn't a relative sentence but a conditional sentence, isn't it? The general rule seems to be: the subordinate clause is separated with a comma from a following main clause. No comma separation is needed if the subordinate clause follows the main clause. Thanks, that's a lesson I learned! > I think the same holds for 'because', 'while' etc. > E.g., jvmtiEnvBase.cpp:1313, jvmtiImpl.cpp:646ff, > vframe_hp.hpp 104ff Ok. I've removed quite a lot of the occurrances. > Also, I like full sentences in comments. > Especially for me as foreign speaker, this makes > things much more clear. I.e., I try to make it > a real sentence with articles, capitalized and a > dot at the end if there is a subject and a verb > in first place. > E.g., jvmtiEnvBase.cpp:1327 Are you referring to the following? (from http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.6/src/hotspot/share/prims/jvmtiEnvBase.cpp.frames.html) 1326 1327 // If the frame is a compiled one, need to deoptimize it. 1328 if (vf->is_compiled_frame()) { This line 1327 is preexisting. > In many places, your comments read really > well but some are quite abbreviated I think. Yeah, but not only because I'm lazy... It is the style that I prefer and I think it matches the surrounding code quite well. > E.g. thread.cpp:2601 is an example where a simple > 'a' helps a lot. > "Single deoptimization is typically very short." > I would add 'A': "A single deoptimization is typically very short (fast?)." > An other meaning of the comment I first considered is this: > "Single deoptimization is typically very short, all_threads deoptimization > takes longer" > having in mind the functions > EscapeBarries::deoptimize_objects_all_threads() > and > EscapeBarries::deoptimize_objects() doing a single thread. > German with it's compound nouns is helpful here :) > Einzeldeoptimierung <--> eine einzelne Deoptimierung I've added the 'A' and I'll try to use complete sentences in the future. The telegram style has advantages, too, though ;) Thanks! Cheers, Richard. -Original Message- From: Lindenmaier, Goetz Sent: Freitag, 17. Juli 2020 14:31 To: Lindenmaier, Goetz ; Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, > I'll answer to the obvious things in this mail now. > I'll go through the code thoroughly again and write > a review of my findings thereafter. As promised a detailed walk-throug, but without any major findings: c1_IR.hpp: ok ci_Env.h|cpp: ok compiledMethod.cpp, nmethod.cpp: ok debugInfoRec.h|cpp: ok scopeDesc.h|cpp ok compileBroker.h|cpp: Maybe a bit of documentation how and why you start the threads? I had expected there are two test scenarios run after each other, but now I understand 'Single' and 'All' run simultaneously. Well, this really is a stress test! Also good the two variants of depotimization are stressed against each other. Besides that really nice it's all in one place. rootResolver.cpp: ok jvmciCodeInstaller.cpp: ok c2compiler.cpp: The essence of this change! Just one line :) Great! callnode.hpp ok escape.h|cpp ok macro.cpp I was not that happy with the names saying not_global_escape and similar. I now agreed you have to use the terms of the escape analysis (NoEscape ArgEscape= throughout the runtime code. I'm still not happy with the 'not' in the term, I always try to expand the name to some sentence with a negated verb, b
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, Thanks for the quick reply. > > > With DeoptimizeObjectsALot enabled internal threads are started that > > > deoptimize frames and > > > objects. The number of threads started are given with > > > DeoptimizeObjectsALotThreadCountAll and > > > DeoptimizeObjectsALotThreadCountSingle. The former targets all > existing > > > threads whereas the > > > latter operates on a single thread selected round robin. > > > > > > I removed the mode where deoptimizations were performed at every nth > > > exit from the runtime. I never used it. > > > Do I get it right? You have a n:1 and a n:all test scenario. > > n:1: n threads deoptimize 1 Jana threadwhere n => DOALThreadCountSingle > > n:m: n threads deoptimize all Java threads where n = DOALThreadCountAll? > > Not quite. > > -XX:+DeoptimizeObjectsALot // required > -XX:DeoptimizeObjectsALotThreadCountAll=m > -XX:DeoptimizeObjectsALotThreadCountSingle=n > > Will start m+n threads. Each operating on all existing JavaThreads using > EscapeBarriers. The > difference between the 2 thread types is that one distinct EscapeBarrier > targets either just a > single thread or all exisitng threads at onece. If just one single thread is > targeted per > EscapeBarrier, then it is not always the same thread, but threads are selected > round robin. So there > will be n threads selecting independently single threads round robin per > EscapeBarrier and m threads > that target all threads in every EscapeBarrier. Ok, yes, that is how I understood it. > > > * EscapeBarrier::sync_and_suspend_one(): use a direct handshake and > > > execute it always independently > > > of is_thread_fully_suspended(). > > Is this also a performance optimization? > > Maybe a minor one. OK > > > * JavaThread::wait_for_object_deoptimization(): > > > - Bugfix: the last check of is_obj_deopt_suspend() must be /after/ the > > > safepoint check! This > > > caused issues with not walkable stacks with DeoptimizeObjectsALot. > > OK. As I understand, there was one safepoint check in the old version, > > now there is one in each iteration. I assume this is intended, right? > > Yes it is. The important thing here is (A) a safepoint check is needed /after/ > leaving a safe state > (_thread_in_native, _thread_blocked). (B) Shared variables that are modified > at safepoints or with handshakes need to be reread /after/ the safepoint > check. > > BTW: I only noticed now that since JDK-8240918 JavaThreads themselves > must disarm their polling > page. Originally (before handshakes) this was done by the VM thread. With > handshakes it was done by > the thread executing the handshake op. This was changed for > OrderAccess::cross_modify_fence() where > the poll is left armed if the thread is in native and sice JDK-8240918 it is > always left armed. So > when a thread leaves a safe state (native, blocked) and there was a > handshake/vm op, it will always > call SafepointMechanism::block_if_requested_slow(), even if the > handshake/vm operation have been > processed already and everybody else is happyly executing bytecodes :) Ok. > Still (A) and (B) hold. > > > - Added limited spinning inspired by HandshakeSpinYield to fix > > > regression in > > > microbenchmark [1] > > Ok. Nice improvement, nice catch! > > Yes. It certainly took some time to find out. > > > > > > > I refer to some more changes answering your questions and comments > inline > > > below. > > > > > > Thanks, > > > Richard. > > > > > > [1] Microbenchmark: > > > > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.6.microbe > nchmark/ > > > > > > > > > I understand you annotate at safepoints where the escape analysis > > > > finds out that an object is "better" than global escape. > > > > This are the cases where the analysis identifies optimization > > > > opportunities. These annotations are then used to deoptimize > > > > frames and the objects referenced by them. > > > > Doesn't this overestimate the optimized > > > > objects? E.g., eliminate_alloc_node has many cases where it bails > > > > out. > > > > > > Yes, the implementation is conservative, but it is comparatively simple > and > > > the additional debug > > > info is just 2 flags per safepoint. > > Thanks. It also helped that you explained to me offline that > > there are more optimizations than only lock elimination and scalar > > replacement done based on the ea information. > > The ea refines the IR graph with allows follow up optimizations > > which can not easily be tracked back to the escaping objects or > > the call sites where they do not escape. > > Thus, if there are non-global escaping objects, you have to > > deoptimize the frame. > > Did I repeat that correctly? > > Mostly, but there are also cases where deoptimization is required if and only > if ea-local objects > are passed as arguments. This is the case when values are not read directly > from a frame, but from a callee frame. Hmm, don't get this completely, but ok.
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
th, deoptimize frames with any optimized objects. > > > // From depth to entry_frame, deoptimize only frames that > > > // pass optimized objects to their callees. > > > (First part similar for the comment above > > EscapeBarrier::deoptimize_objects_internal().) > > > > I've reworked the comment. Let me know if you still think it needs to be > > improved. > Good now, thanks (maybe break the long line ...) Ok. Will do in next webrev.7 > > > Syncronization: looks good. I think others had a look at this before. > > > > > > EscapeBarrier::deoptimize_objects_internal() > > > The method name is misleading, it is not used by > > > deoptimize_objects(). > > > Also, method with the same name is in Deopitmization. > > > Proposal: deoptimize_objects_thread() ? > > > > Sorry, but I don't see, why it would be misleading. > > What would be the meaning of 'deoptimize_objects_thread'? I don't > > understand that name. > 1. I have no idea why it's called "_internal". Because it is private? >By the name, I would expect that EscapeBarrier::deoptimize_objects() >calls it for some internal tasks. But it does not. Well, I'd say it is pretty internal, what's happening in that method. So IMHO the suffix _internal is a match. > 2. My proposal: deoptimize_objects_all_threads() iterates all threads > and calls deoptimize_objects(_one)_thread(thread) for each of these. > That's how I would have named it. > But no bike shedding, if you don't see what I mean it's not obvious. Ok. We could have a quick call, too, if you like. > > > Renaming deferred_locals to deferred_updates is good, as well as > > > adding a datastructure for it. > > > (Adding this data structure might be a breakout, too.) > > > > > > good. > > > > > > thread.cpp > > > > > > good. > > > > > > vframe.cpp > > > > > > Is this a bug in existing code? > > > Makes sense. > > > > Depends on your definition of bug. There are no references to > > vframe::is_entry_frame() in the > > existing code. I would think it is a bug. > So it is :) I'm just afraid it could get fixed by removing the class entryVFrame. > > > > > > > > vframe_hp.hpp > > > (What stands _hp for? helper? The file should be named > > compiledVFrame ...) > > > > > > not_global_escape_in_scope() ... > > > Again, you mention escape analysis here. Comments above hold, too. > > > > I think it is the right name, because it is meaningful and simple. > Ok, accepted ... given my understandings from above. Ok. > > > > > You introduce JvmtiDeferredUpdates. Good. > > > > > > vframe_hp.cpp > > > > > > Changes for JvmtiDeferredUpdates, escape state accessors, > > > > > > line 422: > > > Would an assertion assert(!info->owner_is_scalar_replaced(), ...) hold > > > here? > > > > > > > > > macros.hpp > > > Good. > > > > > > > > > Test coding > > > > > > > > > compileBroker.h|cpp > > > > > > You introduce a third class of threads handled here and > > > add a new flag to distinguish it. Before, the two kinds > > > of threads were distinguished implicitly by passing in > > > a compiler for compiler threads. > > > The new thread kind is only used for testing in debug. > > > > > > make_thread: > > > You could assert (comp != NULL...) to assure previous > > > conditions. > > > > If replaced the if-statements with a switch-statement, made sure all enum- > > elements are covered, and > > added the assertion you suggested. > > > > > line 989 indentation broken > > > > You are referring to this block I assume: > > (from > > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5/src/hots > > pot/share/compiler/compileBroker.cpp.frames.html) > > > > 976 if (MethodFlushing) { > > 977 // Initialize the sweeper thread > > 978 Handle thread_oop = create_thread_oop("Sweeper thread", CHECK); > > 979 jobject thread_handle = JNIHandles::make_local(THREAD, > > thread_oop()); > > 980 make_thread(sweeper_t, thread_handle, NULL, NULL, THREAD); > > 981 } > > 982 > > 983 #if defined(ASSERT) && COMPILER2_OR_JVMCI > > 984 if (DeoptimizeObjectsALot == 2) { > > 985 // Initialize and sta
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, > I'll answer to the obvious things in this mail now. > I'll go through the code thoroughly again and write > a review of my findings thereafter. As promised a detailed walk-throug, but without any major findings: c1_IR.hpp: ok ci_Env.h|cpp: ok compiledMethod.cpp, nmethod.cpp: ok debugInfoRec.h|cpp: ok scopeDesc.h|cpp ok compileBroker.h|cpp: Maybe a bit of documentation how and why you start the threads? I had expected there are two test scenarios run after each other, but now I understand 'Single' and 'All' run simultaneously. Well, this really is a stress test! Also good the two variants of depotimization are stressed against each other. Besides that really nice it's all in one place. rootResolver.cpp: ok jvmciCodeInstaller.cpp: ok c2compiler.cpp: The essence of this change! Just one line :) Great! callnode.hpp ok escape.h|cpp ok macro.cpp I was not that happy with the names saying not_global_escape and similar. I now agreed you have to use the terms of the escape analysis (NoEscape ArgEscape= throughout the runtime code. I'm still not happy with the 'not' in the term, I always try to expand the name to some sentence with a negated verb, but it makes no sense. For example, "has_not_global_escape_in_scope" expands to "Hasn't a global escape in its scope." in my thinking, which makes no sense. You probably mean "Has not-global escape in its scope." or "Has {ArgEscape|NoEscape} in its scope." C2 is using the word "non" in this context, e.g., here alloc->is_non_escaping. non obviously negates the adjective 'global', non-global or nonglobal even is a English term I find in the net. So what about "has_non_global_escape_in_scope?" matcher.cpp ok output.cpp:1071 Please break the long line. jvmtiCodeBlobEvents.cpp ok jvmtiEnv.cpp MaxJavaStackTraceDepth is only documented to affect the exceptions stack trace depth, not to limit jvmti operations. Therefore I wondered why it is used here. Non of your business, but the flag should document this in globals.hpp, too. Does jvmti specify that the same limits are used ...? ok on your side. jvmtiEnvBase.cpp ok jvmtiImpl.h|cpp ok jvmtiTagMap.cpp ok whitebox.cpp ok deoptimization.cpp line 177: Please break line line 246, 281: Please break line 1578, 1583, 1589, 1632, 1649, 1651 Break line 1651: You use 'non'-terms, too: non-escaping :) 2805, 2929, 2946ff, break lines deoptimization.hpp 158, 174, 176 ... I would break lines too, but here you are in good company :) globals.hpp ok mutexLocker.h|cpp ok objectMonitor.cpp ok thread.cpp 2631 typo: sapfepont --> safepoint thread.hpp ok thread.inline.hpp ok vframe.cpp ok vframe_hp.cpp 458ff break lines vframe_hp.hpp ok macros.hpp ok TEST.ROOT ok WhiteBox.java ok IterateHeapWithEscapeAnalysisEnabled.java line 415: msg("wait until target thread has set testMethod_result"); while (testMethod_result == 0) { Thread.sleep(50); } Might the test run into timeouts at this place? The field is volatile, i.e. it will be reloaded in each iteration. But will dontinline_testMethod write it back to main memory in time? libIterateHeapWithEscapeAnalysisEnabled.c ok EATests.java This is a very elaborate test. I found a row of test cases illustrating issues we talked about before. Really helpful! 1311: TypeO materialize -> materialized 1640: setting local variable i triggers always deoptimization --> setting local variable i always triggers deoptimization 2176: dontinline_calee --> dontinline_callee 2510: poping --> popping ... but I'm not sure here. https://www.urbandictionary.com/define.php?term=poping poping Drinking large amounts of Dextromethorphan Hydrobromide (DXM)based cough syrup, and then embarking on an adventure while wandering around neighborhoods or parks all night. This is usually done while listening to Punk rock music from a portable jambox. ;) Don’t do it! 😊 EATestsJVMTI.java I think you can just copy this test description into the other test. You can have two @test comments, they will be treated as separate tests. The @requires will be evaluated accordingly. For an example see test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NullPointerExceptionTest.java which has two different compile setups for the test class (-g). so, that's it for reading code ... Some general remarks, maybe a bit picky ...: I think you could use less commas ',' in comments. As I understand, you need a comma if the relative sentence is at the beginning, but not if it is at the end: If Corona is over, I go to the office. but I go to the office if Corona is over. I think the same holds for 'because', 'while' etc. E.g., jvmtiEnvBase.cpp:1313, jvmtiImpl.cpp:646ff, vframe_hp.hpp 104ff Also, I like full sentences in comments. Especially for me as foreign speaker, this makes things much more clear. I.e., I try to make it a real sentence with articles, capitalized and a dot at the end if there is a subject and a verb in first place. E.g., jvmtiEnvBase.cpp:1327 In many
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, I'll answer to the obvious things in this mail now. I'll go through the code thoroughly again and write a review of my findings thereafter. > So here is the new webrev.6 > > Webrev.6: > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.6/ > Delta: > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.6.inc/ Thanks for the incremental webrev, it's helpful! > I spent most of the time running a microbenchmark [1] I wrote to answer > questions from your > review. At first I had trouble with variance in the results until I found out > it > was due to the NUMA > architecture of the server I used. After that I noticed that there was a > performance regression of > about 5% even at low agent activity. I finally found out that it was due to > the > implementation of > JavaThread::wait_for_object_deoptimization() which is called by the target > of the JVMTI operation to > self suspend for object deoptimization. I fixed this by adding limited > spinning > before calling > wait() on the monitor. > > The delta includes many changes in comments, renaming of names, etc. So > I'd like to summarize > functional changes: > > * Collected all the code for the testing feature DeoptimizeObjectsALot in > compileBroker.cpp and reworked it. Thanks, this makes it much more compact. > With DeoptimizeObjectsALot enabled internal threads are started that > deoptimize frames and > objects. The number of threads started are given with > DeoptimizeObjectsALotThreadCountAll and > DeoptimizeObjectsALotThreadCountSingle. The former targets all existing > threads whereas the > latter operates on a single thread selected round robin. > > I removed the mode where deoptimizations were performed at every nth > exit from the runtime. I never used it. Do I get it right? You have a n:1 and a n:all test scenario. n:1: n threads deoptimize 1 Jana threadwhere n = DOALThreadCountSingle n:m: n threads deoptimize all Java threads where n = DOALThreadCountAll? > * EscapeBarrier::sync_and_suspend_one(): use a direct handshake and > execute it always independently > of is_thread_fully_suspended(). Is this also a performance optimization? > * Bugfix in EscapeBarrier::thread_added(): must not clear deopt flag. Found > this testing with DeoptimizeObjectsALot. Ok. > * Added EscapeBarrier::thread_removed(). Ok. > * EscapeBarrier constructors: barriers can now be entirely disabled by > disabling DoEscapeAnalysis. > This effectively disables the enhancement. Good! > * JavaThread::wait_for_object_deoptimization(): > - Bugfix: the last check of is_obj_deopt_suspend() must be /after/ the > safepoint check! This > caused issues with not walkable stacks with DeoptimizeObjectsALot. OK. As I understand, there was one safepoint check in the old version, now there is one in each iteration. I assume this is intended, right? > - Added limited spinning inspired by HandshakeSpinYield to fix regression in > microbenchmark [1] Ok. Nice improvement, nice catch! > > I refer to some more changes answering your questions and comments inline > below. > > Thanks, > Richard. > > [1] Microbenchmark: > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.6.microbenchmark/ > > > I understand you annotate at safepoints where the escape analysis > > finds out that an object is "better" than global escape. > > This are the cases where the analysis identifies optimization > > opportunities. These annotations are then used to deoptimize > > frames and the objects referenced by them. > > Doesn't this overestimate the optimized > > objects? E.g., eliminate_alloc_node has many cases where it bails > > out. > > Yes, the implementation is conservative, but it is comparatively simple and > the additional debug > info is just 2 flags per safepoint. Thanks. It also helped that you explained to me offline that there are more optimizations than only lock elimination and scalar replacement done based on the ea information. The ea refines the IR graph with allows follow up optimizations which can not easily be tracked back to the escaping objects or the call sites where they do not escape. Thus, if there are non-global escaping objects, you have to deoptimize the frame. Did I repeat that correctly? With this understanding, a row of my proposed renamings/comments are obsolete. > On the other hand, those JVMTI operations > that really trigger > deoptimizations are expected to be comparatively infrequent such that > switching to the interpreter > for a few microseconds will hardly have an effect. That sounds reasonable. > I've done microbenchmarking to check this. > > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.6.microbe > nchmark/ > > I found that in the worst case performance can be impacted by 10%. If the > agent is extremely active > and does relevant JVMTI calls like GetOwnedMonitorStackDepthInfo() every > millisecond or more often, > then the performance impact can be 30%
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
s own.) deoptimize_objects() I would mention escape analysis only as side remark. Also, as I understand, there is only one frame at given depth? // Deoptimize frames with optimized objects. This can be omitted locks and // objects not allocated but replaced by scalars. In C2, these optimizations // are based on escape analysis. // Up to depth, deoptimize frames with any optimized objects. // From depth to entry_frame, deoptimize only frames that // pass optimized objects to their callees. (First part similar for the comment above EscapeBarrier::deoptimize_objects_internal().) What is the check (cur_depth <= depth) good for? Can you ever walk past entry_frame? Isn't vf->is_compiled_frame() prerequisite that "Move to next physical frame" is needed? You could move it into the other check. If so, similar for deoptimize_objects_all_threads(). Syncronization: looks good. I think others had a look at this before. EscapeBarrier::deoptimize_objects_internal() The method name is misleading, it is not used by deoptimize_objects(). Also, method with the same name is in Deopitmization. Proposal: deoptimize_objects_thread() ? C1 stubs: this really shows you tested all configurations, great! mutexLocker: ok. objectMonitor.cpp: ok stackValue.hpp Is this missing clearing a bug? thread.hpp I would remove "_ea" from the flag and method names. Renaming deferred_locals to deferred_updates is good, as well as adding a datastructure for it. (Adding this data structure might be a breakout, too.) good. thread.cpp good. vframe.cpp Is this a bug in existing code? Makes sense. vframe_hp.hpp (What stands _hp for? helper? The file should be named compiledVFrame ...) not_global_escape_in_scope() ... Again, you mention escape analysis here. Comments above hold, too. You introduce JvmtiDeferredUpdates. Good. vframe_hp.cpp Changes for JvmtiDeferredUpdates, escape state accessors, line 422: Would an assertion assert(!info->owner_is_scalar_replaced(), ...) hold here? macros.hpp Good. Test coding compileBroker.h|cpp You introduce a third class of threads handled here and add a new flag to distinguish it. Before, the two kinds of threads were distinguished implicitly by passing in a compiler for compiler threads. The new thread kind is only used for testing in debug. make_thread: You could assert (comp != NULL...) to assure previous conditions. line 989 indentation broken escape.cpp You enable the optimization in case of testruns. good. whitebox.cpp ok. deoptimization.cpp deoptimize_objects_alot_loop() Good. globals.hpp Nice docu of flags, but pleas mention "for testing purposes" or the like in DeoptimizeObjectsALot. I would place the flags next to each other. interfaceSupport.cpp: good. I'll look at the test themselves in an extra mail (learning from Martin 😊) Best regards, Goetz. > -Original Message- > From: Reingruber, Richard > Sent: Wednesday, April 1, 2020 8:15 AM > To: Doerr, Martin ; 'Robbin Ehn' > ; Lindenmaier, Goetz > ; David Holmes ; > Vladimir Kozlov (vladimir.koz...@oracle.com) ; > serviceability-dev@openjdk.java.net; hotspot-compiler- > d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net > Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance > in the Presence of JVMTI Agents > > Hi Martin, > > > thanks for addressing all my points. I've looked over webrev.5 and I'm > satisfied with your changes. > > Thanks! > > > I had also promised to review the tests. > > Thanks++ > I appreciate it very much, the tests are many lines of code. > > > test/jdk/com/sun/jdi/EATests.java > > This is a substantial amount of tests which is appropriate for a such a > > large > change. Skipping some subtests with UseJVMCICompiler makes sense > because it doesn't provide the necessary JVMTI functionality, yet. > > Nice work! > > I also like that you test with and without BiasedLocking. Your tests will > > still > be fine after BiasedLocking deprecation. > > Hope so :) > > > Very minor nits: > > - 2 typos in comment above EARelockingNestedInflatedTarget: "lockes are > ommitted" (sounds funny) > > - You sometimes write "graal" and sometimes "Graal". I guess the capital G > is better. (Also in EATestsJVMCI.java.) > > > test/jdk/com/sun/jdi/EATestsJVMCI.java > > EATests with Graal enabled. Nice that you support Graal to some extent. > Maybe Graal folks want to enhance them in the future. I think this is a good > starting point. > > Will change this in the next webrev. > > > Conclusion: Looks good and not trivial :-) > > Now, you have one full review. I'd be ok with covering 2nd review by partial &
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
> Thanks for cleaning up thread.hpp! Thanks for providing the feedback! I justed noticed that the forward declaration of class jvmtiDeferredLocalVariableSet is not required anymore. Will remove it in the next webrev. Hope to get some more (partial) reviews. Thanks, Richard. -Original Message- From: Robbin Ehn Sent: Dienstag, 31. März 2020 16:21 To: Reingruber, Richard ; Doerr, Martin ; Lindenmaier, Goetz ; David Holmes ; Vladimir Kozlov (vladimir.koz...@oracle.com) ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Thanks for cleaning up thread.hpp! /Robbin On 2020-03-30 10:31, Reingruber, Richard wrote: > Hi, > > this is webrev.5 based on Robbin's feedback and Martin's review - thanks! :) > > The change affects jvmti, hotspot and c2. Partial reviews are very welcome > too. > > Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5/ > Delta: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5.inc/ > > Robbin, Martin, please let me know, if anything shouldn't be quite as you > wanted it. Also find my > comments on your feedback below. > > Robbin, can I count you as Reviewer for the runtime part? > > Thanks, Richard. > > -- > >> DeoptimizeObjectsALotThread is only used in compileBroker.cpp. >> You can move both declaration and definition to that file, no need to clobber >> thread.[c|h]pp. (and the static function deopt_objs_alot_thread_entry) > > Done. > >> Does JvmtiDeferredUpdates really need to be in thread.hpp, can't be in it's >> own >> hpp file? It doesn't seem right to add JVM TI classes into thread.hpp. > > I moved JvmtiDeferredUpdates to vframe_hp.hpp where preexisting > jvmtiDeferredLocalVariableSet is > declared. > >> src/hotspot/share/code/compiledMethod.cpp >> Nice cleanup! > > Thanks :) > >> src/hotspot/share/code/debugInfoRec.cpp >> src/hotspot/share/code/debugInfoRec.hpp >> Additional parmeters. (Remark: I think "non_global_escape_in_scope" would >> read better than "not_global_escape_in_scope", but your version is >> consistent with existing code, so no change request from my side.) Ok. > > I've been thinking about this too and finally stayed with > not_global_escape_in_scope. It's supposed > to mean an object whose escape state is not GlobalEscape is in scope. > >> src/hotspot/share/compiler/compileBroker.cpp >> src/hotspot/share/compiler/compileBroker.hpp >> Extra thread for DeoptimizeObjectsALot. (Remark: I would have put it into a >> follow up change together with the test in order to make this webrev >> smaller, but since it is included, I'm reviewing everything at once. Not a >> big deal.) Ok. > > Yes the change would be a little smaller. And if it helps I'll split it off. > In general I prefer > patches that bring along a suitable amount of tests. > >> src/hotspot/share/opto/c2compiler.cpp >> Make do_escape_analysis independent of JVMCI capabilities. Nice! > > It is the main goal of the enhancement. It is done for C2, but could be done > for JVMCI compilers > with just a small effort as well. > >> src/hotspot/share/opto/escape.cpp >> Annotation for MachSafePointNodes. Your added functionality looks correct. >> But I'd prefer to move the bulky code out of the large function. >> I suggest to factor out something like has_not_global_escape and >> has_arg_escape. So the code could look like this: >>SafePointNode* sfn = sfn_worklist.at(next); >>sfn->set_not_global_escape_in_scope(has_not_global_escape(sfn)); >>if (sfn->is_CallJava()) { >> CallJavaNode* call = sfn->as_CallJava(); >> call->set_arg_escape(has_arg_escape(call)); >>} >> This would also allow us to get rid of the found_..._escape_in_args >> variables making the loops better readable. > > Done. > >> It's kind of ugly to use strcmp to recognize uncommon trap, but that seems >> to be the way to do it (there are more such places). So it's ok. > > Yeah. I copied the snippet. > >> src/hotspot/share/prims/jvmtiImpl.cpp >> src/hotspot/share/prims/jvmtiImpl.hpp >> The sequence is pretty complex: >> VM_GetOrSetLocal element initialization executes EscapeBarrier code which >> suspends the target thread (extra VM Operation). > > Note that the target threads have to be suspended already for > VM_GetOrSetLocal*. So it's mai
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Martin, > thanks for addressing all my points. I've looked over webrev.5 and I'm > satisfied with your changes. Thanks! > I had also promised to review the tests. Thanks++ I appreciate it very much, the tests are many lines of code. > test/jdk/com/sun/jdi/EATests.java > This is a substantial amount of tests which is appropriate for a such a large > change. Skipping some subtests with UseJVMCICompiler makes sense because it > doesn't provide the necessary JVMTI functionality, yet. > Nice work! > I also like that you test with and without BiasedLocking. Your tests will > still be fine after BiasedLocking deprecation. Hope so :) > Very minor nits: > - 2 typos in comment above EARelockingNestedInflatedTarget: "lockes are > ommitted" (sounds funny) > - You sometimes write "graal" and sometimes "Graal". I guess the capital G is > better. (Also in EATestsJVMCI.java.) > test/jdk/com/sun/jdi/EATestsJVMCI.java > EATests with Graal enabled. Nice that you support Graal to some extent. Maybe > Graal folks want to enhance them in the future. I think this is a good > starting point. Will change this in the next webrev. > Conclusion: Looks good and not trivial :-) > Now, you have one full review. I'd be ok with covering 2nd review by partial > reviews. > Compiler and JVMTI parts are not too complicated IMHO. > Runtime part should get at least one additional careful review. Thanks a lot, Richard. -Original Message- From: Doerr, Martin Sent: Dienstag, 31. März 2020 16:01 To: Reingruber, Richard ; 'Robbin Ehn' ; Lindenmaier, Goetz ; David Holmes ; Vladimir Kozlov (vladimir.koz...@oracle.com) ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, thanks for addressing all my points. I've looked over webrev.5 and I'm satisfied with your changes. I had also promised to review the tests. test/hotspot/jtreg/serviceability/jvmti/Heap/IterateHeapWithEscapeAnalysisEnabled.java Thanks for updating the @summary comment. Looks good in webrev.5. test/hotspot/jtreg/serviceability/jvmti/Heap/libIterateHeapWithEscapeAnalysisEnabled.c JVMTI agent for object tagging and heap iteration. Good. test/jdk/com/sun/jdi/EATests.java This is a substantial amount of tests which is appropriate for a such a large change. Skipping some subtests with UseJVMCICompiler makes sense because it doesn't provide the necessary JVMTI functionality, yet. Nice work! I also like that you test with and without BiasedLocking. Your tests will still be fine after BiasedLocking deprecation. Very minor nits: - 2 typos in comment above EARelockingNestedInflatedTarget: "lockes are ommitted" (sounds funny) - You sometimes write "graal" and sometimes "Graal". I guess the capital G is better. (Also in EATestsJVMCI.java.) test/jdk/com/sun/jdi/EATestsJVMCI.java EATests with Graal enabled. Nice that you support Graal to some extent. Maybe Graal folks want to enhance them in the future. I think this is a good starting point. Conclusion: Looks good and not trivial :-) Now, you have one full review. I'd be ok with covering 2nd review by partial reviews. Compiler and JVMTI parts are not too complicated IMHO. Runtime part should get at least one additional careful review. Best regards, Martin > -Original Message- > From: Reingruber, Richard > Sent: Montag, 30. März 2020 10:32 > To: Doerr, Martin ; 'Robbin Ehn' > ; Lindenmaier, Goetz > ; David Holmes ; > Vladimir Kozlov (vladimir.koz...@oracle.com) > ; serviceability-dev@openjdk.java.net; > hotspot-compiler-...@openjdk.java.net; hotspot-runtime- > d...@openjdk.java.net > Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance > in the Presence of JVMTI Agents > > Hi, > > this is webrev.5 based on Robbin's feedback and Martin's review - thanks! :) > > The change affects jvmti, hotspot and c2. Partial reviews are very welcome > too. > > Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5/ > Delta: > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5.inc/ > > Robbin, Martin, please let me know, if anything shouldn't be quite as you > wanted it. Also find my > comments on your feedback below. > > Robbin, can I count you as Reviewer for the runtime part? > > Thanks, Richard. > > -- > > > DeoptimizeObjectsALotThread is only used in compileBroker.cpp. > > You can move both declaration and definition to that file, no need to > clobber > > thread.[c|h]pp. (and the static function deopt_objs_alot_thre
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
ot/share/runtime/deoptimization.hpp Escape barriers and object deoptimization functions. Typo in comment: "helt" => "held" Done in place already. src/hotspot/share/runtime/interfaceSupport.cpp InterfaceSupport::deoptimizeAllObjects() is only used for DeoptimizeObjectsALot = 1. I think DeoptimizeObjectsALot = 2 is more important, but I think it's not bad to have DeoptimizeObjectsALot = 1 in addition. Ok. I never used DeoptimizeObjectsALot = 1 that much. It could be more deterministic in single threaded scenarios. I wouldn't object to get rid of it though. src/hotspot/share/runtime/stackValue.hpp Better reinitilization in StackValue. Good. StackValue::obj_is_scalar_replaced() should not return true after calling set_obj(). src/hotspot/share/runtime/thread.cpp src/hotspot/share/runtime/thread.hpp src/hotspot/share/runtime/thread.inline.hpp wait_for_object_deoptimization, suspend flag, deferred updates and test feature to deoptimize objects. In the long term, we want to get rid of suspend flags, so it's not so nice to introduce a new one. But I agree with Götz that it should be acceptable as temporary solution until async handshakes are available (which takes more time). So I'm ok with your change. I'm keen to build the feature on async handshakes when the arive. You can use MutexLocker with Thread*. Done. JVMTIDeferredUpdates: I agree with Robin. It'd be nice to move the class out of thread.hpp. Done. src/hotspot/share/runtime/vframe.cpp Added support for entry frame to new_vframe. Ok. src/hotspot/share/runtime/vframe_hp.cpp src/hotspot/share/runtime/vframe_hp.hpp I think code()->as_nmethod() in not_global_escape_in_scope() and arg_escape() should better be under #ifdef ASSERT or inside the assert statement (no need for code cache walking in product build). Done. jvmtiDeferredLocalVariableSet::update_monitors: Please add a comment explaining that owner referenced by original info may be scalar replaced, but it is deoptimized in the vframe. Done. -Original Message----- From: Doerr, Martin Sent: Donnerstag, 12. März 2020 17:28 To: Reingruber, Richard ; 'Robbin Ehn' ; Lindenmaier, Goetz ; David Holmes ; Vladimir Kozlov (vladimir.koz...@oracle.com) ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, I managed to find time for a (almost) complete review of webrev.4. (I'll review the tests separately.) First of all, the change seems to be in pretty good quality for its significant complexity. I couldn't find any real bugs. But I'd like to propose minor improvements. I'm convinced that it's mature because we did substantial testing. I like the new functionality for object deoptimization. It can possibly be reused for future escape analysis based optimizations. So I appreciate having it available in the code base. In addition to that, your change makes the JVMTI implementation better integrated into the VM. Now to the details: src/hotspot/share/c1/c1_IR.hpp describe_scope parameters. Ok. src/hotspot/share/ci/ciEnv.cpp src/hotspot/share/ci/ciEnv.hpp Fix for JvmtiExport::can_walk_any_space() capability. Ok. src/hotspot/share/code/compiledMethod.cpp Nice cleanup! src/hotspot/share/code/debugInfoRec.cpp src/hotspot/share/code/debugInfoRec.hpp Additional parmeters. (Remark: I think "non_global_escape_in_scope" would read better than "not_global_escape_in_scope", but your version is consistent with existing code, so no change request from my side.) Ok. src/hotspot/share/code/nmethod.cpp Nice cleanup! src/hotspot/share/code/pcDesc.hpp Additional parameters. Ok. src/hotspot/share/code/scopeDesc.cpp src/hotspot/share/code/scopeDesc.hpp Improved implementation + additional parameters. Ok. src/hotspot/share/compiler/compileBroker.cpp src/hotspot/share/compiler/compileBroker.hpp Extra thread for DeoptimizeObjectsALot. (Remark: I would have put it into a follow up change together with the test in order to make this webrev smaller, but since it is included, I'm reviewing everything at once. Not a big deal.) Ok. src/hotspot/share/jvmci/jvmciCodeInstaller.cpp Additional parameters. Ok. src/hotspot/share/opto/c2compiler.cpp Make do_escape_analysis independent of JVMCI capabilities. Nice! src/hotspot/share/opto/callnode.hpp Additional fields for MachSafePointNodes. Ok. src/hotspot/share/opto/escape.cpp Annotation for MachSafePointNodes. Your added functionality looks correct. But I'd prefer to move the bulky code out of the large function. I suggest to factor out something like has_not_global_escape and has_arg_escape. So the code could look like this: SafePointNode* sfn = sfn_worklist.at(next); sfn->set_not_global_escape_in_scope(
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, thanks for addressing all my points. I've looked over webrev.5 and I'm satisfied with your changes. I had also promised to review the tests. test/hotspot/jtreg/serviceability/jvmti/Heap/IterateHeapWithEscapeAnalysisEnabled.java Thanks for updating the @summary comment. Looks good in webrev.5. test/hotspot/jtreg/serviceability/jvmti/Heap/libIterateHeapWithEscapeAnalysisEnabled.c JVMTI agent for object tagging and heap iteration. Good. test/jdk/com/sun/jdi/EATests.java This is a substantial amount of tests which is appropriate for a such a large change. Skipping some subtests with UseJVMCICompiler makes sense because it doesn't provide the necessary JVMTI functionality, yet. Nice work! I also like that you test with and without BiasedLocking. Your tests will still be fine after BiasedLocking deprecation. Very minor nits: - 2 typos in comment above EARelockingNestedInflatedTarget: "lockes are ommitted" (sounds funny) - You sometimes write "graal" and sometimes "Graal". I guess the capital G is better. (Also in EATestsJVMCI.java.) test/jdk/com/sun/jdi/EATestsJVMCI.java EATests with Graal enabled. Nice that you support Graal to some extent. Maybe Graal folks want to enhance them in the future. I think this is a good starting point. Conclusion: Looks good and not trivial :-) Now, you have one full review. I'd be ok with covering 2nd review by partial reviews. Compiler and JVMTI parts are not too complicated IMHO. Runtime part should get at least one additional careful review. Best regards, Martin > -Original Message- > From: Reingruber, Richard > Sent: Montag, 30. März 2020 10:32 > To: Doerr, Martin ; 'Robbin Ehn' > ; Lindenmaier, Goetz > ; David Holmes ; > Vladimir Kozlov (vladimir.koz...@oracle.com) > ; serviceability-dev@openjdk.java.net; > hotspot-compiler-...@openjdk.java.net; hotspot-runtime- > d...@openjdk.java.net > Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance > in the Presence of JVMTI Agents > > Hi, > > this is webrev.5 based on Robbin's feedback and Martin's review - thanks! :) > > The change affects jvmti, hotspot and c2. Partial reviews are very welcome > too. > > Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5/ > Delta: > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5.inc/ > > Robbin, Martin, please let me know, if anything shouldn't be quite as you > wanted it. Also find my > comments on your feedback below. > > Robbin, can I count you as Reviewer for the runtime part? > > Thanks, Richard. > > -- > > > DeoptimizeObjectsALotThread is only used in compileBroker.cpp. > > You can move both declaration and definition to that file, no need to > clobber > > thread.[c|h]pp. (and the static function deopt_objs_alot_thread_entry) > > Done. > > > Does JvmtiDeferredUpdates really need to be in thread.hpp, can't be in it's > own > > hpp file? It doesn't seem right to add JVM TI classes into thread.hpp. > > I moved JvmtiDeferredUpdates to vframe_hp.hpp where preexisting > jvmtiDeferredLocalVariableSet is > declared. > > > src/hotspot/share/code/compiledMethod.cpp > > Nice cleanup! > > Thanks :) > > > src/hotspot/share/code/debugInfoRec.cpp > > src/hotspot/share/code/debugInfoRec.hpp > > Additional parmeters. (Remark: I think "non_global_escape_in_scope" > would read better than "not_global_escape_in_scope", but your version is > consistent with existing code, so no change request from my side.) Ok. > > I've been thinking about this too and finally stayed with > not_global_escape_in_scope. It's supposed > to mean an object whose escape state is not GlobalEscape is in scope. > > > src/hotspot/share/compiler/compileBroker.cpp > > src/hotspot/share/compiler/compileBroker.hpp > > Extra thread for DeoptimizeObjectsALot. (Remark: I would have put it into > a follow up change together with the test in order to make this webrev > smaller, but since it is included, I'm reviewing everything at once. Not a big > deal.) Ok. > > Yes the change would be a little smaller. And if it helps I'll split it off. > In > general I prefer > patches that bring along a suitable amount of tests. > > > src/hotspot/share/opto/c2compiler.cpp > > Make do_escape_analysis independent of JVMCI capabilities. Nice! > > It is the main goal of the enhancement. It is done for C2, but could be done > for JVMCI compilers > with just a small effort as well. > > > src/hotspot/share/opto/escape.cpp > > Annotation for MachSafePointNodes. Your added functionality looks > correct. &
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
deoptimize_objects functions makes it a little hard to > keep an overview of which one is used for what. > Maybe adding suffixes would help a little bit, but I can also live with what > you have. > Implementation looks correct to me. 2 are internal. I added the suffix _internal to them. This leaves 2 to choose from. > src/hotspot/share/runtime/deoptimization.hpp > Escape barriers and object deoptimization functions. > Typo in comment: "helt" => "held" Done in place already. > src/hotspot/share/runtime/interfaceSupport.cpp > InterfaceSupport::deoptimizeAllObjects() is only used for > DeoptimizeObjectsALot = 1. > I think DeoptimizeObjectsALot = 2 is more important, but I think it's not bad > to have DeoptimizeObjectsALot = 1 in addition. Ok. I never used DeoptimizeObjectsALot = 1 that much. It could be more deterministic in single threaded scenarios. I wouldn't object to get rid of it though. > src/hotspot/share/runtime/stackValue.hpp > Better reinitilization in StackValue. Good. StackValue::obj_is_scalar_replaced() should not return true after calling set_obj(). > src/hotspot/share/runtime/thread.cpp > src/hotspot/share/runtime/thread.hpp > src/hotspot/share/runtime/thread.inline.hpp > wait_for_object_deoptimization, suspend flag, deferred updates and test > feature to deoptimize objects. > In the long term, we want to get rid of suspend flags, so it's not so nice to > introduce a new one. But I agree with Götz that it should be acceptable as > temporary solution until async handshakes are available (which takes more > time). So I'm ok with your change. I'm keen to build the feature on async handshakes when the arive. > You can use MutexLocker with Thread*. Done. > JVMTIDeferredUpdates: I agree with Robin. It'd be nice to move the class out > of thread.hpp. Done. > src/hotspot/share/runtime/vframe.cpp > Added support for entry frame to new_vframe. Ok. > src/hotspot/share/runtime/vframe_hp.cpp > src/hotspot/share/runtime/vframe_hp.hpp > I think code()->as_nmethod() in not_global_escape_in_scope() and arg_escape() > should better be under #ifdef ASSERT or inside the assert statement (no need > for code cache walking in product build). Done. > jvmtiDeferredLocalVariableSet::update_monitors: > Please add a comment explaining that owner referenced by original info may be > scalar replaced, but it is deoptimized in the vframe. Done. -Original Message- From: Doerr, Martin Sent: Donnerstag, 12. März 2020 17:28 To: Reingruber, Richard ; 'Robbin Ehn' ; Lindenmaier, Goetz ; David Holmes ; Vladimir Kozlov (vladimir.koz...@oracle.com) ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, I managed to find time for a (almost) complete review of webrev.4. (I'll review the tests separately.) First of all, the change seems to be in pretty good quality for its significant complexity. I couldn't find any real bugs. But I'd like to propose minor improvements. I'm convinced that it's mature because we did substantial testing. I like the new functionality for object deoptimization. It can possibly be reused for future escape analysis based optimizations. So I appreciate having it available in the code base. In addition to that, your change makes the JVMTI implementation better integrated into the VM. Now to the details: src/hotspot/share/c1/c1_IR.hpp describe_scope parameters. Ok. src/hotspot/share/ci/ciEnv.cpp src/hotspot/share/ci/ciEnv.hpp Fix for JvmtiExport::can_walk_any_space() capability. Ok. src/hotspot/share/code/compiledMethod.cpp Nice cleanup! src/hotspot/share/code/debugInfoRec.cpp src/hotspot/share/code/debugInfoRec.hpp Additional parmeters. (Remark: I think "non_global_escape_in_scope" would read better than "not_global_escape_in_scope", but your version is consistent with existing code, so no change request from my side.) Ok. src/hotspot/share/code/nmethod.cpp Nice cleanup! src/hotspot/share/code/pcDesc.hpp Additional parameters. Ok. src/hotspot/share/code/scopeDesc.cpp src/hotspot/share/code/scopeDesc.hpp Improved implementation + additional parameters. Ok. src/hotspot/share/compiler/compileBroker.cpp src/hotspot/share/compiler/compileBroker.hpp Extra thread for DeoptimizeObjectsALot. (Remark: I would have put it into a follow up change together with the test in order to make this webrev smaller, but since it is included, I'm reviewing everything at once. Not a big deal.) Ok. src/hotspot/share/jvmci/jvmciCodeInstaller.cpp Additional parameters. Ok. src/hotspot/share/opto/c2compiler.cpp Make do_escape_analysis independent of JVMCI c
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi, this is webrev.5 based on Robbin's feedback and Martin's review - thanks! :) The change affects jvmti, hotspot and c2. Partial reviews are very welcome too. Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5/ Delta: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5.inc/ Robbin, Martin, please let me know, if anything shouldn't be quite as you wanted it. Also find my comments on your feedback below. Robbin, can I count you as Reviewer for the runtime part? Thanks, Richard. -Original Message- From: Doerr, Martin Sent: Donnerstag, 12. März 2020 17:28 To: Reingruber, Richard ; 'Robbin Ehn' ; Lindenmaier, Goetz ; David Holmes ; Vladimir Kozlov (vladimir.koz...@oracle.com) ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, I managed to find time for a (almost) complete review of webrev.4. (I'll review the tests separately.) First of all, the change seems to be in pretty good quality for its significant complexity. I couldn't find any real bugs. But I'd like to propose minor improvements. I'm convinced that it's mature because we did substantial testing. I like the new functionality for object deoptimization. It can possibly be reused for future escape analysis based optimizations. So I appreciate having it available in the code base. In addition to that, your change makes the JVMTI implementation better integrated into the VM. Now to the details: src/hotspot/share/c1/c1_IR.hpp describe_scope parameters. Ok. src/hotspot/share/ci/ciEnv.cpp src/hotspot/share/ci/ciEnv.hpp Fix for JvmtiExport::can_walk_any_space() capability. Ok. src/hotspot/share/code/compiledMethod.cpp Nice cleanup! src/hotspot/share/code/debugInfoRec.cpp src/hotspot/share/code/debugInfoRec.hpp Additional parmeters. (Remark: I think "non_global_escape_in_scope" would read better than "not_global_escape_in_scope", but your version is consistent with existing code, so no change request from my side.) Ok. src/hotspot/share/code/nmethod.cpp Nice cleanup! src/hotspot/share/code/pcDesc.hpp Additional parameters. Ok. src/hotspot/share/code/scopeDesc.cpp src/hotspot/share/code/scopeDesc.hpp Improved implementation + additional parameters. Ok. src/hotspot/share/compiler/compileBroker.cpp src/hotspot/share/compiler/compileBroker.hpp Extra thread for DeoptimizeObjectsALot. (Remark: I would have put it into a follow up change together with the test in order to make this webrev smaller, but since it is included, I'm reviewing everything at once. Not a big deal.) Ok. src/hotspot/share/jvmci/jvmciCodeInstaller.cpp Additional parameters. Ok. src/hotspot/share/opto/c2compiler.cpp Make do_escape_analysis independent of JVMCI capabilities. Nice! src/hotspot/share/opto/callnode.hpp Additional fields for MachSafePointNodes. Ok. src/hotspot/share/opto/escape.cpp Annotation for MachSafePointNodes. Your added functionality looks correct. But I'd prefer to move the bulky code out of the large function. I suggest to factor out something like has_not_global_escape and has_arg_escape. So the code could look like this: SafePointNode* sfn = sfn_worklist.at(next); sfn->set_not_global_escape_in_scope(has_not_global_escape(sfn)); if (sfn->is_CallJava()) { CallJavaNode* call = sfn->as_CallJava(); call->set_arg_escape(has_arg_escape(call)); } This would also allow us to get rid of the found_..._escape_in_args variables making the loops better readable. It's kind of ugly to use strcmp to recognize uncommon trap, but that seems to be the way to do it (there are more such places). So it's ok. src/hotspot/share/opto/machnode.hpp Additional fields for MachSafePointNodes. Ok. src/hotspot/share/opto/macro.cpp Allow elimination of non-escaping allocations. Ok. src/hotspot/share/opto/matcher.cpp src/hotspot/share/opto/output.cpp Copy attribute / pass parameters. Ok. src/hotspot/share/prims/jvmtiCodeBlobEvents.cpp Nice cleanup! src/hotspot/share/prims/jvmtiEnv.cpp src/hotspot/share/prims/jvmtiEnvBase.cpp Escape barriers + deoptimize objects for target thread. Good. src/hotspot/share/prims/jvmtiImpl.cpp src/hotspot/share/prims/jvmtiImpl.hpp The sequence is pretty complex: VM_GetOrSetLocal element initialization executes EscapeBarrier code which suspends the target thread (extra VM Operation). VM_GetOrSetLocal::doit_prologue performs object deoptimization (by VM Thread to prepare VM Operation with frame deoptimization). VM_GetOrSetLocal destructor implicitly calls EscapeBarrier destructor which resumes the target thread. But I don't have any improvement proposal. Performance is probably not a concern, here. So it's ok. VM_GetOrSetLocal::deoptim
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Martin, thanks a lot for reviewing and the feedback. I'll dig into the details as soon as possible. Looking forward to it :) Thanks, Richard. -Original Message- From: Doerr, Martin Sent: Donnerstag, 12. März 2020 17:28 To: Reingruber, Richard ; 'Robbin Ehn' ; Lindenmaier, Goetz ; David Holmes ; Vladimir Kozlov (vladimir.koz...@oracle.com) ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, I managed to find time for a (almost) complete review of webrev.4. (I'll review the tests separately.) First of all, the change seems to be in pretty good quality for its significant complexity. I couldn't find any real bugs. But I'd like to propose minor improvements. I'm convinced that it's mature because we did substantial testing. I like the new functionality for object deoptimization. It can possibly be reused for future escape analysis based optimizations. So I appreciate having it available in the code base. In addition to that, your change makes the JVMTI implementation better integrated into the VM. Now to the details: src/hotspot/share/c1/c1_IR.hpp describe_scope parameters. Ok. src/hotspot/share/ci/ciEnv.cpp src/hotspot/share/ci/ciEnv.hpp Fix for JvmtiExport::can_walk_any_space() capability. Ok. src/hotspot/share/code/compiledMethod.cpp Nice cleanup! src/hotspot/share/code/debugInfoRec.cpp src/hotspot/share/code/debugInfoRec.hpp Additional parmeters. (Remark: I think "non_global_escape_in_scope" would read better than "not_global_escape_in_scope", but your version is consistent with existing code, so no change request from my side.) Ok. src/hotspot/share/code/nmethod.cpp Nice cleanup! src/hotspot/share/code/pcDesc.hpp Additional parameters. Ok. src/hotspot/share/code/scopeDesc.cpp src/hotspot/share/code/scopeDesc.hpp Improved implementation + additional parameters. Ok. src/hotspot/share/compiler/compileBroker.cpp src/hotspot/share/compiler/compileBroker.hpp Extra thread for DeoptimizeObjectsALot. (Remark: I would have put it into a follow up change together with the test in order to make this webrev smaller, but since it is included, I'm reviewing everything at once. Not a big deal.) Ok. src/hotspot/share/jvmci/jvmciCodeInstaller.cpp Additional parameters. Ok. src/hotspot/share/opto/c2compiler.cpp Make do_escape_analysis independent of JVMCI capabilities. Nice! src/hotspot/share/opto/callnode.hpp Additional fields for MachSafePointNodes. Ok. src/hotspot/share/opto/escape.cpp Annotation for MachSafePointNodes. Your added functionality looks correct. But I'd prefer to move the bulky code out of the large function. I suggest to factor out something like has_not_global_escape and has_arg_escape. So the code could look like this: SafePointNode* sfn = sfn_worklist.at(next); sfn->set_not_global_escape_in_scope(has_not_global_escape(sfn)); if (sfn->is_CallJava()) { CallJavaNode* call = sfn->as_CallJava(); call->set_arg_escape(has_arg_escape(call)); } This would also allow us to get rid of the found_..._escape_in_args variables making the loops better readable. It's kind of ugly to use strcmp to recognize uncommon trap, but that seems to be the way to do it (there are more such places). So it's ok. src/hotspot/share/opto/machnode.hpp Additional fields for MachSafePointNodes. Ok. src/hotspot/share/opto/macro.cpp Allow elimination of non-escaping allocations. Ok. src/hotspot/share/opto/matcher.cpp src/hotspot/share/opto/output.cpp Copy attribute / pass parameters. Ok. src/hotspot/share/prims/jvmtiCodeBlobEvents.cpp Nice cleanup! src/hotspot/share/prims/jvmtiEnv.cpp src/hotspot/share/prims/jvmtiEnvBase.cpp Escape barriers + deoptimize objects for target thread. Good. src/hotspot/share/prims/jvmtiImpl.cpp src/hotspot/share/prims/jvmtiImpl.hpp The sequence is pretty complex: VM_GetOrSetLocal element initialization executes EscapeBarrier code which suspends the target thread (extra VM Operation). VM_GetOrSetLocal::doit_prologue performs object deoptimization (by VM Thread to prepare VM Operation with frame deoptimization). VM_GetOrSetLocal destructor implicitly calls EscapeBarrier destructor which resumes the target thread. But I don't have any improvement proposal. Performance is probably not a concern, here. So it's ok. VM_GetOrSetLocal::deoptimize_objects deoptimizes the top frame if it has non-globally escaping objects and other frames if they have arg escaping ones. Good. src/hotspot/share/prims/jvmtiTagMap.cpp Escape barriers + deoptimize objects for all threads. Ok. src/hotspot/share/prims/whitebox.cpp Added WB_IsFrameDeoptimized to API. Ok. src/hotspot/share/runtime/deoptimization.cpp Object deoptimiz
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
t was it. Best regards, Martin > -Original Message- > From: hotspot-compiler-dev boun...@openjdk.java.net> On Behalf Of Reingruber, Richard > Sent: Dienstag, 3. März 2020 21:23 > To: 'Robbin Ehn' ; Lindenmaier, Goetz > ; David Holmes ; > Vladimir Kozlov (vladimir.koz...@oracle.com) > ; serviceability-dev@openjdk.java.net; > hotspot-compiler-...@openjdk.java.net; hotspot-runtime- > d...@openjdk.java.net > Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better > Performance in the Presence of JVMTI Agents > > Hi Robbin, > > > > I understand that Robbin proposed to replace the usage of > > > _suspend_flag with handshakes. Apparently, async handshakes > > > are needed to do so. We have been waiting a while for removal > > > of the _suspend_flag / introduction of async handshakes [2]. > > > What is the status here? > > > I have an old prototype which I would like to continue to work on. > > So do not assume asynch handshakes will make 15. > > Even if it would, I think there are a lot more investigate work to remove > > _suspend_flag. > > Let us know, if we can be of any help to you and be it only testing. > > > >> Full: > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4/ > > > DeoptimizeObjectsALotThread is only used in compileBroker.cpp. > > You can move both declaration and definition to that file, no need to > clobber > > thread.[c|h]pp. (and the static function deopt_objs_alot_thread_entry) > > Will do. > > > Does JvmtiDeferredUpdates really need to be in thread.hpp, can't be in it's > own > > hpp file? It doesn't seem right to add JVM TI classes into thread.hpp. > > You are right. It shouldn't be declared in thread.hpp. I will look into that. > > > Note that we also think we may have a bug in deopt: > > https://bugs.openjdk.java.net/browse/JDK-8238237 > > > I think it would be best, if possible, to push after that is resolved. > > Sure. > > > Not even nearly a full review :) > > I know :) > > Anyways, thanks a lot, > Richard. > > > -Original Message- > From: Robbin Ehn > Sent: Monday, March 2, 2020 11:17 AM > To: Lindenmaier, Goetz ; Reingruber, Richard > ; David Holmes ; > Vladimir Kozlov (vladimir.koz...@oracle.com) > ; serviceability-dev@openjdk.java.net; > hotspot-compiler-...@openjdk.java.net; hotspot-runtime- > d...@openjdk.java.net > Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance > in the Presence of JVMTI Agents > > Hi, > > On 2/24/20 5:39 PM, Lindenmaier, Goetz wrote: > > Hi, > > > > I had a look at the progress of this change. Nothing > > happened since Richard posted his update using more > > handshakes [1]. > > But we (SAP) would appreciate a lot if this change could > > be successfully reviewed and pushed. > > > > I think there is basic understanding that this > > change is helpful. It fixes a number of issues with JVMTI, > > and will deliver the same performance benefits as EA > > does in current production mode for debugging scenarios. > > > > This is important for us as we run our VMs prepared > > for debugging in production mode. > > > > I understand that Robbin proposed to replace the usage of > > _suspend_flag with handshakes. Apparently, async handshakes > > are needed to do so. We have been waiting a while for removal > > of the _suspend_flag / introduction of async handshakes [2]. > > What is the status here? > > I have an old prototype which I would like to continue to work on. > So do not assume asynch handshakes will make 15. > Even if it would, I think there are a lot more investigate work to remove > _suspend_flag. > > > > > I think we should no longer wait, but proceed with > > this change. We will look into removing the usage of > > suspend_flag introduced here once it is possible to implement > > it with handshakes. > > Yes, sure. > > >> Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4/ > > DeoptimizeObjectsALotThread is only used in compileBroker.cpp. > You can move both declaration and definition to that file, no need to clobber > thread.[c|h]pp. (and the static function deopt_objs_alot_thread_entry) > > Does JvmtiDeferredUpdates really need to be in thread.hpp, can't be in it's > own > hpp file? It doesn't seem right to add JVM TI classes into thread.hpp. > > Note that we also think we may have a bug in deopt: > https://bugs.openjdk.java.net/browse/JDK-8238237 > > I think it would be best,
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Robbin, > > I understand that Robbin proposed to replace the usage of > > _suspend_flag with handshakes. Apparently, async handshakes > > are needed to do so. We have been waiting a while for removal > > of the _suspend_flag / introduction of async handshakes [2]. > > What is the status here? > I have an old prototype which I would like to continue to work on. > So do not assume asynch handshakes will make 15. > Even if it would, I think there are a lot more investigate work to remove > _suspend_flag. Let us know, if we can be of any help to you and be it only testing. > >> Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4/ > DeoptimizeObjectsALotThread is only used in compileBroker.cpp. > You can move both declaration and definition to that file, no need to clobber > thread.[c|h]pp. (and the static function deopt_objs_alot_thread_entry) Will do. > Does JvmtiDeferredUpdates really need to be in thread.hpp, can't be in it's > own > hpp file? It doesn't seem right to add JVM TI classes into thread.hpp. You are right. It shouldn't be declared in thread.hpp. I will look into that. > Note that we also think we may have a bug in deopt: > https://bugs.openjdk.java.net/browse/JDK-8238237 > I think it would be best, if possible, to push after that is resolved. Sure. > Not even nearly a full review :) I know :) Anyways, thanks a lot, Richard. -Original Message- From: Robbin Ehn Sent: Monday, March 2, 2020 11:17 AM To: Lindenmaier, Goetz ; Reingruber, Richard ; David Holmes ; Vladimir Kozlov (vladimir.koz...@oracle.com) ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi, On 2/24/20 5:39 PM, Lindenmaier, Goetz wrote: > Hi, > > I had a look at the progress of this change. Nothing > happened since Richard posted his update using more > handshakes [1]. > But we (SAP) would appreciate a lot if this change could > be successfully reviewed and pushed. > > I think there is basic understanding that this > change is helpful. It fixes a number of issues with JVMTI, > and will deliver the same performance benefits as EA > does in current production mode for debugging scenarios. > > This is important for us as we run our VMs prepared > for debugging in production mode. > > I understand that Robbin proposed to replace the usage of > _suspend_flag with handshakes. Apparently, async handshakes > are needed to do so. We have been waiting a while for removal > of the _suspend_flag / introduction of async handshakes [2]. > What is the status here? I have an old prototype which I would like to continue to work on. So do not assume asynch handshakes will make 15. Even if it would, I think there are a lot more investigate work to remove _suspend_flag. > > I think we should no longer wait, but proceed with > this change. We will look into removing the usage of > suspend_flag introduced here once it is possible to implement > it with handshakes. Yes, sure. >> Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4/ DeoptimizeObjectsALotThread is only used in compileBroker.cpp. You can move both declaration and definition to that file, no need to clobber thread.[c|h]pp. (and the static function deopt_objs_alot_thread_entry) Does JvmtiDeferredUpdates really need to be in thread.hpp, can't be in it's own hpp file? It doesn't seem right to add JVM TI classes into thread.hpp. Note that we also think we may have a bug in deopt: https://bugs.openjdk.java.net/browse/JDK-8238237 I think it would be best, if possible, to push after that is resolved. Not even nearly a full review :) Thanks, Robbin >> Incremental: >> http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4.inc/ >> >> I was not able to eliminate the additional suspend flag now. I'll take care >> of this >> as soon as the >> existing suspend-resume-mechanism is reworked. >> >> Testing: >> >> Nightly tests @SAP: >> >>JCK and JTREG, also in Xcomp mode, SPECjvm2008, SPECjbb2015, Renaissance >> Suite, SAP specific tests >>with fastdebug and release builds on all platforms >> >>Stress testing with DeoptimizeObjectsALot running SPECjvm2008 40x parallel >> for 24h >> >> Thanks, Richard. >> >> >> More details on the changes: >> >> * Hide DeoptimizeObjectsALotThread from external view. >> >> * Changed EscapeBarrier_lock to be a _safepoint_check_never lock. >>It used to be _safepoint_check_sometimes, which will be eliminated sooner >> or >
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi, On 2/24/20 5:39 PM, Lindenmaier, Goetz wrote: Hi, I had a look at the progress of this change. Nothing happened since Richard posted his update using more handshakes [1]. But we (SAP) would appreciate a lot if this change could be successfully reviewed and pushed. I think there is basic understanding that this change is helpful. It fixes a number of issues with JVMTI, and will deliver the same performance benefits as EA does in current production mode for debugging scenarios. This is important for us as we run our VMs prepared for debugging in production mode. I understand that Robbin proposed to replace the usage of _suspend_flag with handshakes. Apparently, async handshakes are needed to do so. We have been waiting a while for removal of the _suspend_flag / introduction of async handshakes [2]. What is the status here? I have an old prototype which I would like to continue to work on. So do not assume asynch handshakes will make 15. Even if it would, I think there are a lot more investigate work to remove _suspend_flag. I think we should no longer wait, but proceed with this change. We will look into removing the usage of suspend_flag introduced here once it is possible to implement it with handshakes. Yes, sure. Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4/ DeoptimizeObjectsALotThread is only used in compileBroker.cpp. You can move both declaration and definition to that file, no need to clobber thread.[c|h]pp. (and the static function deopt_objs_alot_thread_entry) Does JvmtiDeferredUpdates really need to be in thread.hpp, can't be in it's own hpp file? It doesn't seem right to add JVM TI classes into thread.hpp. Note that we also think we may have a bug in deopt: https://bugs.openjdk.java.net/browse/JDK-8238237 I think it would be best, if possible, to push after that is resolved. Not even nearly a full review :) Thanks, Robbin Incremental: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4.inc/ I was not able to eliminate the additional suspend flag now. I'll take care of this as soon as the existing suspend-resume-mechanism is reworked. Testing: Nightly tests @SAP: JCK and JTREG, also in Xcomp mode, SPECjvm2008, SPECjbb2015, Renaissance Suite, SAP specific tests with fastdebug and release builds on all platforms Stress testing with DeoptimizeObjectsALot running SPECjvm2008 40x parallel for 24h Thanks, Richard. More details on the changes: * Hide DeoptimizeObjectsALotThread from external view. * Changed EscapeBarrier_lock to be a _safepoint_check_never lock. It used to be _safepoint_check_sometimes, which will be eliminated sooner or later. I added explicit thread state changes with ThreadBlockInVM to code paths where we can wait() on EscapeBarrier_lock to become safepoint safe. * Use handshake EscapeBarrierSuspendHandshake to suspend target threads instead of vm operation VM_ThreadSuspendAllForObjDeopt. * Removed uses of Threads_lock. When adding a new thread we suspend it iff EA optimizations are being reverted. In the previous version we were waiting on Threads_lock while EA optimizations were reverted. See EscapeBarrier::thread_added(). * Made tests require Xmixed compilation mode. * Made tests agnostic regarding tiered compilation. I.e. tc isn't disabled anymore, and the tests can be run with tc enabled or disabled. * Exercising EATests.java as well with stress test options DeoptimizeObjectsALot* Due to the non-deterministic deoptimizations some tests need to be skipped. We do this to prevent bit-rot of the stress test code. * Executing EATests.java as well with graal if available. Driver for this is EATestsJVMCI.java. Graal cannot pass all tests, because it does not provide all the new debug info (namely not_global_escape_in_scope and arg_escape in scopeDesc.hpp). And graal does not yet support the JVMTI operations force early return and pop frame. * Removed tracing from new jdi tests in EATests.java. Too much trace output before the debugging connection is established can cause deadlock because output buffers fill up. (See https://bugs.openjdk.java.net/browse/JDK-8173304) * Many copyright year changes and smaller clean-up changes of testing code (trailing white-space and the like). -Original Message- From: David Holmes Sent: Donnerstag, 19. Dezember 2019 03:12 To: Reingruber, Richard ; serviceability- d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot- runtime-...@openjdk.java.net; Vladimir Kozlov (vladimir.koz...@oracle.com) Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, I think my issue is with the way EliminateNestedLocks works so I'm going to look into that more deeply. Thanks for the explanations. David On 18/12/2019 12:47 am, Reingruber, Richard wrote: Hi David, > >> Some further queries/
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi, I had a look at the progress of this change. Nothing happened since Richard posted his update using more handshakes [1]. But we (SAP) would appreciate a lot if this change could be successfully reviewed and pushed. I think there is basic understanding that this change is helpful. It fixes a number of issues with JVMTI, and will deliver the same performance benefits as EA does in current production mode for debugging scenarios. This is important for us as we run our VMs prepared for debugging in production mode. I understand that Robbin proposed to replace the usage of _suspend_flag with handshakes. Apparently, async handshakes are needed to do so. We have been waiting a while for removal of the _suspend_flag / introduction of async handshakes [2]. What is the status here? I think we should no longer wait, but proceed with this change. We will look into removing the usage of suspend_flag introduced here once it is possible to implement it with handshakes. Also, I think it's a good point in time to push this, as jdk15 is at the beginning of development. Best regards, Goetz. [1] http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-February/037984.html [2] http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2019-December/037533.html > -Original Message- > From: hotspot-runtime-dev > On Behalf Of Reingruber, Richard > Sent: Dienstag, 4. Februar 2020 09:59 > To: David Holmes ; Vladimir Kozlov > (vladimir.koz...@oracle.com) ; Robbin Ehn > ; serviceability-dev@openjdk.java.net; hotspot- > compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net > Subject: [CAUTION] RE: RFR(L) 8227745: Enable Escape Analysis for Better > Performance in the Presence of JVMTI Agents > > Hi, > > I have prepared webrev.4 that incorporates feedback from webrev.3 (thanks!) > > Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4/ > Incremental: > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4.inc/ > > I was not able to eliminate the additional suspend flag now. I'll take care > of this > as soon as the > existing suspend-resume-mechanism is reworked. > > Testing: > > Nightly tests @SAP: > > JCK and JTREG, also in Xcomp mode, SPECjvm2008, SPECjbb2015, Renaissance > Suite, SAP specific tests > with fastdebug and release builds on all platforms > > Stress testing with DeoptimizeObjectsALot running SPECjvm2008 40x parallel > for 24h > > Thanks, Richard. > > > More details on the changes: > > * Hide DeoptimizeObjectsALotThread from external view. > > * Changed EscapeBarrier_lock to be a _safepoint_check_never lock. > It used to be _safepoint_check_sometimes, which will be eliminated sooner or > later. > I added explicit thread state changes with ThreadBlockInVM to code paths > where we can wait() > on EscapeBarrier_lock to become safepoint safe. > > * Use handshake EscapeBarrierSuspendHandshake to suspend target threads > instead of vm operation > VM_ThreadSuspendAllForObjDeopt. > > * Removed uses of Threads_lock. When adding a new thread we suspend it iff > EA optimizations are > being reverted. In the previous version we were waiting on Threads_lock > while EA optimizations > were reverted. See EscapeBarrier::thread_added(). > > * Made tests require Xmixed compilation mode. > > * Made tests agnostic regarding tiered compilation. > I.e. tc isn't disabled anymore, and the tests can be run with tc enabled or > disabled. > > * Exercising EATests.java as well with stress test options > DeoptimizeObjectsALot* > Due to the non-deterministic deoptimizations some tests need to be skipped. > We do this to prevent bit-rot of the stress test code. > > * Executing EATests.java as well with graal if available. Driver for this is > EATestsJVMCI.java. Graal cannot pass all tests, because it does not provide > all > the new debug info > (namely not_global_escape_in_scope and arg_escape in scopeDesc.hpp). > And graal does not yet support the JVMTI operations force early return and > pop frame. > > * Removed tracing from new jdi tests in EATests.java. Too much trace output > before the debugging > connection is established can cause deadlock because output buffers fill up. > (See https://bugs.openjdk.java.net/browse/JDK-8173304) > > * Many copyright year changes and smaller clean-up changes of testing code > (trailing white-space and > the like). > > > -Original Message----- > From: David Holmes > Sent: Donnerstag, 19. Dezember 2019 03:12 > To: Reingruber, Richard ; serviceability- > d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot- > runtime-...@openjdk.java.net; Vladimir
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Ping :) Richard. -Original Message- From: hotspot-compiler-dev On Behalf Of Reingruber, Richard Sent: Dienstag, 4. Februar 2020 09:59 To: David Holmes ; Vladimir Kozlov (vladimir.koz...@oracle.com) ; Robbin Ehn ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: [CAUTION] RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi, I have prepared webrev.4 that incorporates feedback from webrev.3 (thanks!) Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4/ Incremental: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4.inc/ I was not able to eliminate the additional suspend flag now. I'll take care of this as soon as the existing suspend-resume-mechanism is reworked. Testing: Nightly tests @SAP: JCK and JTREG, also in Xcomp mode, SPECjvm2008, SPECjbb2015, Renaissance Suite, SAP specific tests with fastdebug and release builds on all platforms Stress testing with DeoptimizeObjectsALot running SPECjvm2008 40x parallel for 24h Thanks, Richard. More details on the changes: * Hide DeoptimizeObjectsALotThread from external view. * Changed EscapeBarrier_lock to be a _safepoint_check_never lock. It used to be _safepoint_check_sometimes, which will be eliminated sooner or later. I added explicit thread state changes with ThreadBlockInVM to code paths where we can wait() on EscapeBarrier_lock to become safepoint safe. * Use handshake EscapeBarrierSuspendHandshake to suspend target threads instead of vm operation VM_ThreadSuspendAllForObjDeopt. * Removed uses of Threads_lock. When adding a new thread we suspend it iff EA optimizations are being reverted. In the previous version we were waiting on Threads_lock while EA optimizations were reverted. See EscapeBarrier::thread_added(). * Made tests require Xmixed compilation mode. * Made tests agnostic regarding tiered compilation. I.e. tc isn't disabled anymore, and the tests can be run with tc enabled or disabled. * Exercising EATests.java as well with stress test options DeoptimizeObjectsALot* Due to the non-deterministic deoptimizations some tests need to be skipped. We do this to prevent bit-rot of the stress test code. * Executing EATests.java as well with graal if available. Driver for this is EATestsJVMCI.java. Graal cannot pass all tests, because it does not provide all the new debug info (namely not_global_escape_in_scope and arg_escape in scopeDesc.hpp). And graal does not yet support the JVMTI operations force early return and pop frame. * Removed tracing from new jdi tests in EATests.java. Too much trace output before the debugging connection is established can cause deadlock because output buffers fill up. (See https://bugs.openjdk.java.net/browse/JDK-8173304) * Many copyright year changes and smaller clean-up changes of testing code (trailing white-space and the like). -Original Message- From: David Holmes Sent: Donnerstag, 19. Dezember 2019 03:12 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; Vladimir Kozlov (vladimir.koz...@oracle.com) Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, I think my issue is with the way EliminateNestedLocks works so I'm going to look into that more deeply. Thanks for the explanations. David On 18/12/2019 12:47 am, Reingruber, Richard wrote: > Hi David, > >> >> Some further queries/concerns: >> >> >> >> src/hotspot/share/runtime/objectMonitor.cpp >> >> >> >> Can you please explain the changes to ObjectMonitor::wait: >> >> >> >> ! _recursions = save // restore the old recursion count >> >> ! + jt->get_and_reset_relock_count_after_wait(); > // >> >> increased by the deferred relock count >> >> >> >> what is the "deferred relock count"? I gather it relates to >> >> >> >> "The code was extended to be able to deoptimize objects of a >> > frame that >> >> is not the top frame and to let another thread than the owning >> > thread do >> >> it." >> > >> > Yes, these relate. Currently EA based optimizations are reverted, when > a compiled frame is >> > replaced with corresponding interpreter frames. Part of this is > relocking objects with eliminated >> > locking. New with the enhancement is that we do this also just before > object references are >> > acqui
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi, I have prepared webrev.4 that incorporates feedback from webrev.3 (thanks!) Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4/ Incremental: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4.inc/ I was not able to eliminate the additional suspend flag now. I'll take care of this as soon as the existing suspend-resume-mechanism is reworked. Testing: Nightly tests @SAP: JCK and JTREG, also in Xcomp mode, SPECjvm2008, SPECjbb2015, Renaissance Suite, SAP specific tests with fastdebug and release builds on all platforms Stress testing with DeoptimizeObjectsALot running SPECjvm2008 40x parallel for 24h Thanks, Richard. More details on the changes: * Hide DeoptimizeObjectsALotThread from external view. * Changed EscapeBarrier_lock to be a _safepoint_check_never lock. It used to be _safepoint_check_sometimes, which will be eliminated sooner or later. I added explicit thread state changes with ThreadBlockInVM to code paths where we can wait() on EscapeBarrier_lock to become safepoint safe. * Use handshake EscapeBarrierSuspendHandshake to suspend target threads instead of vm operation VM_ThreadSuspendAllForObjDeopt. * Removed uses of Threads_lock. When adding a new thread we suspend it iff EA optimizations are being reverted. In the previous version we were waiting on Threads_lock while EA optimizations were reverted. See EscapeBarrier::thread_added(). * Made tests require Xmixed compilation mode. * Made tests agnostic regarding tiered compilation. I.e. tc isn't disabled anymore, and the tests can be run with tc enabled or disabled. * Exercising EATests.java as well with stress test options DeoptimizeObjectsALot* Due to the non-deterministic deoptimizations some tests need to be skipped. We do this to prevent bit-rot of the stress test code. * Executing EATests.java as well with graal if available. Driver for this is EATestsJVMCI.java. Graal cannot pass all tests, because it does not provide all the new debug info (namely not_global_escape_in_scope and arg_escape in scopeDesc.hpp). And graal does not yet support the JVMTI operations force early return and pop frame. * Removed tracing from new jdi tests in EATests.java. Too much trace output before the debugging connection is established can cause deadlock because output buffers fill up. (See https://bugs.openjdk.java.net/browse/JDK-8173304) * Many copyright year changes and smaller clean-up changes of testing code (trailing white-space and the like). -Original Message- From: David Holmes Sent: Donnerstag, 19. Dezember 2019 03:12 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; Vladimir Kozlov (vladimir.koz...@oracle.com) Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, I think my issue is with the way EliminateNestedLocks works so I'm going to look into that more deeply. Thanks for the explanations. David On 18/12/2019 12:47 am, Reingruber, Richard wrote: > Hi David, > >> >> Some further queries/concerns: >> >> >> >> src/hotspot/share/runtime/objectMonitor.cpp >> >> >> >> Can you please explain the changes to ObjectMonitor::wait: >> >> >> >> ! _recursions = save // restore the old recursion count >> >> ! + jt->get_and_reset_relock_count_after_wait(); > // >> >> increased by the deferred relock count >> >> >> >> what is the "deferred relock count"? I gather it relates to >> >> >> >> "The code was extended to be able to deoptimize objects of a >> > frame that >> >> is not the top frame and to let another thread than the owning >> > thread do >> >> it." >> > >> > Yes, these relate. Currently EA based optimizations are reverted, when > a compiled frame is >> > replaced with corresponding interpreter frames. Part of this is > relocking objects with eliminated >> > locking. New with the enhancement is that we do this also just before > object references are >> > acquired through JVMTI. In this case we deoptimize also the owning > compiled frame C and we >> > register deoptimized objects as deferred updates. When control returns > to C it gets deoptimized, >> > we notice that objects are already deoptimized (reallocated and > relocked), so we don't do it again >> > (relocking twice would be incorrect of course). Deferred updates are > copied into the new &g
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi, webrev.3 didn't apply anymore after 8236000 [1]. I've rebased and updated in place: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/ The change was minimal. Cheers, Richard. [1] JDK-8236000: VM build without C2 fails -Original Message- From: Reingruber, Richard Sent: Dienstag, 10. Dezember 2019 22:45 To: serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi, I would like to get reviews please for http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/ Corresponding RFE: https://bugs.openjdk.java.net/browse/JDK-8227745 Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915 And potentially https://bugs.openjdk.java.net/browse/JDK-8214584 [1] Vladimir Kozlov kindly put webrev.3 through tier1-8 testing without issues (thanks!). In addition the change is being tested at SAP since I posted the first RFR some months ago. The intention of this enhancement is to benefit performance wise from escape analysis even if JVMTI agents request capabilities that allow them to access local variable values. E.g. if you start-up with -agentlib:jdwp=transport=dt_socket,server=y,suspend=n, then escape analysis is disabled right from the beginning, well before a debugger attaches -- if ever one should do so. With the enhancement, escape analysis will remain enabled until and after a debugger attaches. EA based optimizations are reverted just before an agent acquires the reference to an object. In the JBS item you'll find more details. Thanks, Richard. [1] Experimental fix for JDK-8214584 based on JDK-8227745 http://cr.openjdk.java.net/~rrich/webrevs/2019/8214584/experiment_v1.patch
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
ocalObject l1 = new LocalObject(); in front of the synchrnized blocks and assume a JVM TI agent acquires l1. This triggers the code in question. See that relocking/reallocating is transactional. If it is done then for /all/ objects in scope and it is done at most once. It wouldn't be quite so easy to split this in relocking of nested/EA-based eliminated locks. > If so I find this truly awful. Anyone using wait() in a realistic form > requires a notification and so the object cannot be thread confined. In It is not thread confined. > which case I would strongly argue that upon hitting the wait() the deopt > should occur unconditionally and so the lock state is correct before we > wait and so we don't need to mess with the recursion count internally > when we reacquire the monitor. > > > > >> which I don't like the sound of at all when it comes to ObjectMonitor > >> state. So I'd like to understand in detail exactly what is going on here > >> and why. This is a very intrusive change that seems to badly break > >> encapsulation and impacts future changes to ObjectMonitor that are under > >> investigation. > > > > I would not regard this as breaking encapsulation. Certainly not badly. > > > > I've added a property relock_count_after_wait to JavaThread. The property is well > > encapsulated. Future ObjectMonitor implementations have to deal with recursion too. They are free > > in choosing a way to do that as long as that property is taken into account. This is hardly a > > limitation. > > I do think this badly breaks encapsulation as you have to add a callout > from the guts of the ObjectMonitor code to reach into the thread to get > this lock count adjustment. I understand why you have had to do this but > I would much rather see a change to the EA optimisation strategy so that > this is not needed. > > > Note also that the property is a straight forward extension of the existing concept of deferred > > local updates. It is embedded into the structure holding them. So not even the footprint of a > > JavaThread is enlarged if no deferred updates are generated. > > [...] > > > > > I'm actually duplicating the existing external suspend mechanism, because a thread can be > > suspended at most once. And hey, and don't like that either! But it seems not unlikely that the > > duplicate can be removed together with the original and the new type of handshakes that will be > > used for thread suspend can be used for object deoptimization too. See today's discussion in > > JDK-8227745 [2]. > > I hope that discussion bears some fruit, at the moment it seems not to > be possible to use handshakes here. :( > > The external suspend mechanism is a royal pain in the proverbial that we > have to carefully live with. The idea that we're duplicating that for > use in another fringe area of functionality does not thrill me at all. > > To be clear, I understand the problem that exists and that you wish to > solve, but for the runtime parts I balk at the complexity cost of > solving it. I know it's complex, but by far no rocket science. Also I find it hard to imagine another fix for JDK-8233915 besides changing the JVM TI specification. Thanks, Richard. -Original Message- From: David Holmes Sent: Dienstag, 17. Dezember 2019 08:03 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; Vladimir Kozlov (vladimir.koz...@oracle.com) Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents David On 17/12/2019 4:57 pm, David Holmes wrote: Hi Richard, On 14/12/2019 5:01 am, Reingruber, Richard wrote: Hi David, > Some further queries/concerns: > > src/hotspot/share/runtime/objectMonitor.cpp > > Can you please explain the changes to ObjectMonitor::wait: > > ! _recursions = save // restore the old recursion count > ! + jt->get_and_reset_relock_count_after_wait(); // > increased by the deferred relock count > > what is the "deferred relock count"? I gather it relates to > > "The code was extended to be able to deoptimize objects of a frame that > is not the top frame and to let another thread than the owning thread do > it." Yes, these relate. Currently EA based optimizations are reverted, when a compiled frame is replaced with corresponding interprete
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
is done at most once. It wouldn't be quite so easy to split this in relocking of nested/EA-based eliminated locks. > If so I find this truly awful. Anyone using wait() in a realistic form > requires a notification and so the object cannot be thread confined. In It is not thread confined. > which case I would strongly argue that upon hitting the wait() the deopt > should occur unconditionally and so the lock state is correct before we > wait and so we don't need to mess with the recursion count internally > when we reacquire the monitor. > > > > >> which I don't like the sound of at all when it comes to ObjectMonitor > >> state. So I'd like to understand in detail exactly what is going on here > >> and why. This is a very intrusive change that seems to badly break > >> encapsulation and impacts future changes to ObjectMonitor that are under > >> investigation. > > > > I would not regard this as breaking encapsulation. Certainly not badly. > > > > I've added a property relock_count_after_wait to JavaThread. The property is well > > encapsulated. Future ObjectMonitor implementations have to deal with recursion too. They are free > > in choosing a way to do that as long as that property is taken into account. This is hardly a > > limitation. > > I do think this badly breaks encapsulation as you have to add a callout > from the guts of the ObjectMonitor code to reach into the thread to get > this lock count adjustment. I understand why you have had to do this but > I would much rather see a change to the EA optimisation strategy so that > this is not needed. > > > Note also that the property is a straight forward extension of the existing concept of deferred > > local updates. It is embedded into the structure holding them. So not even the footprint of a > > JavaThread is enlarged if no deferred updates are generated. > > [...] > > > > > I'm actually duplicating the existing external suspend mechanism, because a thread can be > > suspended at most once. And hey, and don't like that either! But it seems not unlikely that the > > duplicate can be removed together with the original and the new type of handshakes that will be > > used for thread suspend can be used for object deoptimization too. See today's discussion in > > JDK-8227745 [2]. > > I hope that discussion bears some fruit, at the moment it seems not to > be possible to use handshakes here. :( > > The external suspend mechanism is a royal pain in the proverbial that we > have to carefully live with. The idea that we're duplicating that for > use in another fringe area of functionality does not thrill me at all. > > To be clear, I understand the problem that exists and that you wish to > solve, but for the runtime parts I balk at the complexity cost of > solving it. I know it's complex, but by far no rocket science. Also I find it hard to imagine another fix for JDK-8233915 besides changing the JVM TI specification. Thanks, Richard. -Original Message- From: David Holmes Sent: Dienstag, 17. Dezember 2019 08:03 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; Vladimir Kozlov (vladimir.koz...@oracle.com) Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents David On 17/12/2019 4:57 pm, David Holmes wrote: > Hi Richard, > > On 14/12/2019 5:01 am, Reingruber, Richard wrote: >> Hi David, >> >> > Some further queries/concerns: >> > >> > src/hotspot/share/runtime/objectMonitor.cpp >> > >> > Can you please explain the changes to ObjectMonitor::wait: >> > >> > ! _recursions = save // restore the old recursion count >> > ! + jt->get_and_reset_relock_count_after_wait(); // >> > increased by the deferred relock count >> > >> > what is the "deferred relock count"? I gather it relates to >> > >> > "The code was extended to be able to deoptimize objects of a >> frame that >> > is not the top frame and to let another thread than the owning >> thread do >> > it." >> >> Yes, these relate. Currently EA based optimizations are reverted, when >> a compiled frame is replaced >> with corresponding interpreter frames. Part of this is relocking >> objects with eliminated &
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, On 12/17/19 11:24 AM, Reingruber, Richard wrote: > So adding asynch handshakes and a per thread handshake queue, we can. > (which this test prototype does) Yes, should work for my use case too. Great. > The issue I'm thinking of is if we need selective polling first. > Suspend flags are not checked in every transition, e.g. vm->native. > A JVM TI agent don't expect to suspend it's own thread when suspending > all threads. > (that thread would be suspended when trying to get back to agent code > when it does vm->native transition) Note that JVM TI doesn't offer "suspending all threads" directly. It offers SuspendThreadList [1] which can be used to self-suspend: "If the calling thread is specified in the request_list array, this function will not return until some other thread resumes it" Maybe there is a test-bug here or it was more complicated scenario. I have to investigate, but suspending threads in all transitions causes a chunk of test failure in jdi/jvmti. The issue was suspending threads going vm->native (back to agent code). Thanks, Robbin Thanks, Richard. [1] https://docs.oracle.com/en/java/javase/13/docs/specs/jvmti.html#SuspendThreadList -Original Message- From: Robbin Ehn Sent: Montag, 16. Dezember 2019 18:21 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, On 2019-12-16 14:41, Reingruber, Richard wrote: Hi Robbin, first of all: thanks a lot for providing feedback. I do appreciate it. I am absolutely willing to move this to handshakes. Only I still can't see how to achieve it. Could you explain the drafted class EscapeBarrierSuspendHandshake a little bit? [1] I'd like to look at it by example of JvmtiEnv::GetOwnedMonitorStackDepthInfo() where calling_thread T1 would apply it on another thread T2. Sorry I don't immediately see what issue there is in doing a handshake instead of: VM_GetOwnedMonitorInfo op(this, calling_thread, java_thread, owned_monitors_list); 1. L13: is wait_until_eb_stopped to be called by T1 to wait until T2 cannot move anymore? 2. Handshakes between two threads are synchronous, correct? If so, then T1 will block handshaking T2, because either T2 or the VMThread will block in L10. Yes, sorry, I forgot/confused myself about asynch handshake. (I have a test prototype for that, which removes suspend flag) I cannot figure out, how you mean this. Only if a helper thread H would handshake T2 then T1 could continue and call wait_until_eb_stopped(). But returning from there T1 would block if reallocating objects triggers GC or attempting to execute the vm operation in JvmtiEnv::GetOwnedMonitorStackDepthInfo(). It might be impossible to replace my suspend flag with handshakes that are available today, because if it was you could replace all the suspend flags right away, couldn't you? So adding asynch handshakes and a per thread handshake queue, we can. (which this test prototype does) The issue I'm thinking of is if we need selective polling first. Suspend flags are not checked in every transition, e.g. vm->native. A JVM TI agent don't expect to suspend it's own thread when suspending all threads. (that thread would be suspended when trying to get back to agent code when it does vm->native transition) Or I'm simply missing something... quite possible... :) No I think you got it right. Thanks, Robbin Thanks, Richard. [1] Drafted by Robbin (thanks!) 1class EscapeBarrierSuspendHandshake : public HandshakeClosure { 2 Semaphore _is_waiting; 3 Semaphore _wait; 4 bool _started; 5public: 6 EscapeBarrierSuspendHandshake() : HandshakeClosure("EscapeBarrierSuspend"), 7 _wait(0), _started(false) { } 8 void do_thread(Thread* th) { 9_is_waiting.signal(); 10_wait.wait(); 11Atomic::store(&_started, true); 12 } 13 void wait_until_eb_stopped() { _is_waiting.wait(); } 14 void start_thread() { 15_wait.signal(); 16while(!Atomic::load(&_started)) { 17 os::naked_yield(); 18} 19 } 20}; -Original Message- From: Robbin Ehn Sent: Montag, 16. Dezember 2019 11:21 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, as m
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Robbin, > Sorry I don't immediately see what issue there is in doing a handshake > instead of: > VM_GetOwnedMonitorInfo op(this, calling_thread, java_thread, > owned_monitors_list); VM_GetOwnedMonitorInfo /can/ be replaced by a handshake, but the calling_thread T1 needs to walk java_thread T2's stack /before/ to reallocate and relock objects, because the GC interface does not allow the VMThread to allocate from the java heap. T1: 1. reallocate scalar replaced objects of T2 // not possible as part of handshake/vmop, // because GC interface does not allow VMThread // to allocate from heap 2. execute VM_GetOwnedMonitorInfo() or equivalent handshake while T2 is /not/ pushing new frames with EA based optimizations. > > > > 1. L13: is wait_until_eb_stopped to be called by T1 to wait until T2 cannot move anymore? > > > > 2. Handshakes between two threads are synchronous, correct? If so, then T1 will block handshaking > > T2, because either T2 or the VMThread will block in L10. > > Yes, sorry, I forgot/confused myself about asynch handshake. > (I have a test prototype for that, which removes suspend flag) > > > > > I cannot figure out, how you mean this. Only if a helper thread H would handshake T2 then T1 could > > continue and call wait_until_eb_stopped(). But returning from there T1 would block if reallocating > > objects triggers GC or attempting to execute the vm operation in > > JvmtiEnv::GetOwnedMonitorStackDepthInfo(). > > > > It might be impossible to replace my suspend flag with handshakes that are available today, because > > if it was you could replace all the suspend flags right away, couldn't you? > > So adding asynch handshakes and a per thread handshake queue, we can. > (which this test prototype does) Yes, should work for my use case too. > The issue I'm thinking of is if we need selective polling first. > Suspend flags are not checked in every transition, e.g. vm->native. > A JVM TI agent don't expect to suspend it's own thread when suspending > all threads. > (that thread would be suspended when trying to get back to agent code > when it does vm->native transition) Note that JVM TI doesn't offer "suspending all threads" directly. It offers SuspendThreadList [1] which can be used to self-suspend: "If the calling thread is specified in the request_list array, this function will not return until some other thread resumes it" Thanks, Richard. [1] https://docs.oracle.com/en/java/javase/13/docs/specs/jvmti.html#SuspendThreadList -Original Message- From: Robbin Ehn Sent: Montag, 16. Dezember 2019 18:21 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, On 2019-12-16 14:41, Reingruber, Richard wrote: > Hi Robbin, > > first of all: thanks a lot for providing feedback. I do appreciate it. > > I am absolutely willing to move this to handshakes. Only I still can't see > how to achieve it. > > Could you explain the drafted class EscapeBarrierSuspendHandshake a little > bit? [1] > > I'd like to look at it by example of > JvmtiEnv::GetOwnedMonitorStackDepthInfo() where calling_thread > T1 would apply it on another thread T2. Sorry I don't immediately see what issue there is in doing a handshake instead of: VM_GetOwnedMonitorInfo op(this, calling_thread, java_thread, owned_monitors_list); > > 1. L13: is wait_until_eb_stopped to be called by T1 to wait until T2 cannot > move anymore? > > 2. Handshakes between two threads are synchronous, correct? If so, then T1 > will block handshaking > T2, because either T2 or the VMThread will block in L10. Yes, sorry, I forgot/confused myself about asynch handshake. (I have a test prototype for that, which removes suspend flag) > > I cannot figure out, how you mean this. Only if a helper thread H would > handshake T2 then T1 could > continue and call wait_until_eb_stopped(). But returning from there T1 would > block if reallocating > objects triggers GC or attempting to execute the vm operation in > JvmtiEnv::GetOwnedMonitorStackDepthInfo(). > > It might be impossible to replace my suspend flag with handshakes that are > available today, because > if it was you could replace all the suspend flags right away, couldn't you? So adding asynch handshakes and a per thread handshake queue, we can. (which this test prototype does) The iss
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
ly). This seems like it may be something > that handshakes could be used for. Deopt suspend used to be something rather different with a similar name[1]. It is not being added back. I stand corrected. Despite comments in the code to the contrary deopt_suspend didn't actually cause a self-suspend. I was doing a lot of cleanup in this area 13 years ago :) I'm actually duplicating the existing external suspend mechanism, because a thread can be suspended at most once. And hey, and don't like that either! But it seems not unlikely that the duplicate can be removed together with the original and the new type of handshakes that will be used for thread suspend can be used for object deoptimization too. See today's discussion in JDK-8227745 [2]. I hope that discussion bears some fruit, at the moment it seems not to be possible to use handshakes here. :( The external suspend mechanism is a royal pain in the proverbial that we have to carefully live with. The idea that we're duplicating that for use in another fringe area of functionality does not thrill me at all. To be clear, I understand the problem that exists and that you wish to solve, but for the runtime parts I balk at the complexity cost of solving it. Thanks, David - Thanks, Richard. [1] Deopt suspend was something like an async. handshake for architectures with register windows, where patching the return pc for deoptimization of a compiled frame was racy if the owner thread was in native code. Instead a "deopt" suspend flag was set on which the thread patched its own frame upon return from native. So no thread was suspended. It got its name only from the name of the flags. [2] Discussion about using handshakes to sync. with the target thread: https://bugs.openjdk.java.net/browse/JDK-8227745?focusedCommentId=14306727&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14306727 -Original Message- From: David Holmes Sent: Freitag, 13. Dezember 2019 00:56 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, Some further queries/concerns: src/hotspot/share/runtime/objectMonitor.cpp Can you please explain the changes to ObjectMonitor::wait: ! _recursions = save // restore the old recursion count ! + jt->get_and_reset_relock_count_after_wait(); // increased by the deferred relock count what is the "deferred relock count"? I gather it relates to "The code was extended to be able to deoptimize objects of a frame that is not the top frame and to let another thread than the owning thread do it." which I don't like the sound of at all when it comes to ObjectMonitor state. So I'd like to understand in detail exactly what is going on here and why. This is a very intrusive change that seems to badly break encapsulation and impacts future changes to ObjectMonitor that are under investigation. --- src/hotspot/share/runtime/thread.cpp Can you please explain why JavaThread::wait_for_object_deoptimization has to be handcrafted in this way rather than using proper transitions. We got rid of "deopt suspend" some time ago and it is disturbing to see it being added back (effectively). This seems like it may be something that handshakes could be used for. Thanks, David - On 12/12/2019 7:02 am, David Holmes wrote: On 12/12/2019 1:07 am, Reingruber, Richard wrote: Hi David, > Most of the details here are in areas I can comment on in detail, but I > did take an initial general look at things. Thanks for taking the time! Apologies the above should read: "Most of the details here are in areas I *can't* comment on in detail ..." David > The only thing that jumped out at me is that I think the > DeoptimizeObjectsALotThread should be a hidden thread. > > + bool is_hidden_from_external_view() const { return true; } Yes, it should. Will add the method like above. > Also I don't see any testing of the DeoptimizeObjectsALotThread. Without > active testing this will just bit-rot. DeoptimizeObjectsALot is meant for stress testing with a larger workload. I will add a minimal test to keep it fresh. > Also on the tests I don't understand your @requires clause: > > @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & > (vm.opt.TieredCompilation != true)) > > This seems to require that TieredCompilation is disabled, but tiered is > our normal mode of operation. ?? > I removed the clause. I guess I wanted to target the tests towards the code they are supposed to test, and it'
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, On 2019-12-16 14:41, Reingruber, Richard wrote: Hi Robbin, first of all: thanks a lot for providing feedback. I do appreciate it. I am absolutely willing to move this to handshakes. Only I still can't see how to achieve it. Could you explain the drafted class EscapeBarrierSuspendHandshake a little bit? [1] I'd like to look at it by example of JvmtiEnv::GetOwnedMonitorStackDepthInfo() where calling_thread T1 would apply it on another thread T2. Sorry I don't immediately see what issue there is in doing a handshake instead of: VM_GetOwnedMonitorInfo op(this, calling_thread, java_thread, owned_monitors_list); 1. L13: is wait_until_eb_stopped to be called by T1 to wait until T2 cannot move anymore? 2. Handshakes between two threads are synchronous, correct? If so, then T1 will block handshaking T2, because either T2 or the VMThread will block in L10. Yes, sorry, I forgot/confused myself about asynch handshake. (I have a test prototype for that, which removes suspend flag) I cannot figure out, how you mean this. Only if a helper thread H would handshake T2 then T1 could continue and call wait_until_eb_stopped(). But returning from there T1 would block if reallocating objects triggers GC or attempting to execute the vm operation in JvmtiEnv::GetOwnedMonitorStackDepthInfo(). It might be impossible to replace my suspend flag with handshakes that are available today, because if it was you could replace all the suspend flags right away, couldn't you? So adding asynch handshakes and a per thread handshake queue, we can. (which this test prototype does) The issue I'm thinking of is if we need selective polling first. Suspend flags are not checked in every transition, e.g. vm->native. A JVM TI agent don't expect to suspend it's own thread when suspending all threads. (that thread would be suspended when trying to get back to agent code when it does vm->native transition) Or I'm simply missing something... quite possible... :) No I think you got it right. Thanks, Robbin Thanks, Richard. [1] Drafted by Robbin (thanks!) 1 class EscapeBarrierSuspendHandshake : public HandshakeClosure { 2 Semaphore _is_waiting; 3 Semaphore _wait; 4 bool _started; 5 public: 6 EscapeBarrierSuspendHandshake() : HandshakeClosure("EscapeBarrierSuspend"), 7 _wait(0), _started(false) { } 8 void do_thread(Thread* th) { 9 _is_waiting.signal(); 10 _wait.wait(); 11 Atomic::store(&_started, true); 12 } 13 void wait_until_eb_stopped() { _is_waiting.wait(); } 14 void start_thread() { 15 _wait.signal(); 16 while(!Atomic::load(&_started)) { 17 os::naked_yield(); 18 } 19 } 20 }; -Original Message- From: Robbin Ehn Sent: Montag, 16. Dezember 2019 11:21 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, as mentioned it would be better if you could do this with handshakes, instead of using _suspend_flag (since they are going away). But I can't think of a way doing it without blocking safepoints, so we need to add some more features in handshakes first. When possible I hope you are willing to move this code to handshakes instead. You could stop one thread with, e.g.: class EscapeBarrierSuspendHandshake : public HandshakeClosure { Semaphore _is_waiting; Semaphore _wait; bool _started; public: EscapeBarrierSuspendHandshake() : HandshakeClosure("EscapeBarrierSuspend"), _wait(0), _started(false) { } void do_thread(Thread* th) { _is_waiting.signal(); _wait.wait(); Atomic::store(&_started, true); } void wait_until_eb_stopped() { _is_waiting.wait(); } void start_thread() { _wait.signal(); while(!Atomic::load(&_started)) { os::naked_yield(); } } }; But it would block safepoints. Thanks, Robbin On 12/10/19 10:45 PM, Reingruber, Richard wrote: Hi, I would like to get reviews please for http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/ Corresponding RFE: https://bugs.openjdk.java.net/browse/JDK-8227745 Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915 And potentially https://bugs.openjdk.java.net/browse/JDK-8214584 [1] Vladimir Kozlov kindly put webrev.3 through tier1-8 testing without issues (thanks!). In addition the change is being tested at SAP since I posted the first RFR some months ago. The intention of this enhancement is to benefit performance wise from escape analysis even if JVMTI agents request capabilities that allow them to access local variable values. E.g. if you start-up with -agentlib:jdwp=transport=dt_socket,serv
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Robbin, first of all: thanks a lot for providing feedback. I do appreciate it. I am absolutely willing to move this to handshakes. Only I still can't see how to achieve it. Could you explain the drafted class EscapeBarrierSuspendHandshake a little bit? [1] I'd like to look at it by example of JvmtiEnv::GetOwnedMonitorStackDepthInfo() where calling_thread T1 would apply it on another thread T2. 1. L13: is wait_until_eb_stopped to be called by T1 to wait until T2 cannot move anymore? 2. Handshakes between two threads are synchronous, correct? If so, then T1 will block handshaking T2, because either T2 or the VMThread will block in L10. I cannot figure out, how you mean this. Only if a helper thread H would handshake T2 then T1 could continue and call wait_until_eb_stopped(). But returning from there T1 would block if reallocating objects triggers GC or attempting to execute the vm operation in JvmtiEnv::GetOwnedMonitorStackDepthInfo(). It might be impossible to replace my suspend flag with handshakes that are available today, because if it was you could replace all the suspend flags right away, couldn't you? Or I'm simply missing something... quite possible... :) Thanks, Richard. [1] Drafted by Robbin (thanks!) 1 class EscapeBarrierSuspendHandshake : public HandshakeClosure { 2Semaphore _is_waiting; 3Semaphore _wait; 4bool _started; 5 public: 6EscapeBarrierSuspendHandshake() : HandshakeClosure("EscapeBarrierSuspend"), 7_wait(0), _started(false) { } 8void do_thread(Thread* th) { 9 _is_waiting.signal(); 10 _wait.wait(); 11 Atomic::store(&_started, true); 12} 13void wait_until_eb_stopped() { _is_waiting.wait(); } 14void start_thread() { 15 _wait.signal(); 16 while(!Atomic::load(&_started)) { 17os::naked_yield(); 18 } 19} 20 }; -Original Message- From: Robbin Ehn Sent: Montag, 16. Dezember 2019 11:21 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, as mentioned it would be better if you could do this with handshakes, instead of using _suspend_flag (since they are going away). But I can't think of a way doing it without blocking safepoints, so we need to add some more features in handshakes first. When possible I hope you are willing to move this code to handshakes instead. You could stop one thread with, e.g.: class EscapeBarrierSuspendHandshake : public HandshakeClosure { Semaphore _is_waiting; Semaphore _wait; bool _started; public: EscapeBarrierSuspendHandshake() : HandshakeClosure("EscapeBarrierSuspend"), _wait(0), _started(false) { } void do_thread(Thread* th) { _is_waiting.signal(); _wait.wait(); Atomic::store(&_started, true); } void wait_until_eb_stopped() { _is_waiting.wait(); } void start_thread() { _wait.signal(); while(!Atomic::load(&_started)) { os::naked_yield(); } } }; But it would block safepoints. Thanks, Robbin On 12/10/19 10:45 PM, Reingruber, Richard wrote: > Hi, > > I would like to get reviews please for > > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/ > > Corresponding RFE: > https://bugs.openjdk.java.net/browse/JDK-8227745 > > Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915 > And potentially https://bugs.openjdk.java.net/browse/JDK-8214584 [1] > > Vladimir Kozlov kindly put webrev.3 through tier1-8 testing without issues > (thanks!). In addition the > change is being tested at SAP since I posted the first RFR some months ago. > > The intention of this enhancement is to benefit performance wise from escape > analysis even if JVMTI > agents request capabilities that allow them to access local variable values. > E.g. if you start-up > with -agentlib:jdwp=transport=dt_socket,server=y,suspend=n, then escape > analysis is disabled right > from the beginning, well before a debugger attaches -- if ever one should do > so. With the > enhancement, escape analysis will remain enabled until and after a debugger > attaches. EA based > optimizations are reverted just before an agent acquires the reference to an > object. In the JBS item > you'll find more details. > > Thanks, > Richard. > > [1] Experimental fix for JDK-8214584 based on JDK-8227745 > > http://cr.openjdk.java.net/~rrich/webrevs/2019/8214584/experiment_v1.patch >
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, as mentioned it would be better if you could do this with handshakes, instead of using _suspend_flag (since they are going away). But I can't think of a way doing it without blocking safepoints, so we need to add some more features in handshakes first. When possible I hope you are willing to move this code to handshakes instead. You could stop one thread with, e.g.: class EscapeBarrierSuspendHandshake : public HandshakeClosure { Semaphore _is_waiting; Semaphore _wait; bool _started; public: EscapeBarrierSuspendHandshake() : HandshakeClosure("EscapeBarrierSuspend"), _wait(0), _started(false) { } void do_thread(Thread* th) { _is_waiting.signal(); _wait.wait(); Atomic::store(&_started, true); } void wait_until_eb_stopped() { _is_waiting.wait(); } void start_thread() { _wait.signal(); while(!Atomic::load(&_started)) { os::naked_yield(); } } }; But it would block safepoints. Thanks, Robbin On 12/10/19 10:45 PM, Reingruber, Richard wrote: Hi, I would like to get reviews please for http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/ Corresponding RFE: https://bugs.openjdk.java.net/browse/JDK-8227745 Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915 And potentially https://bugs.openjdk.java.net/browse/JDK-8214584 [1] Vladimir Kozlov kindly put webrev.3 through tier1-8 testing without issues (thanks!). In addition the change is being tested at SAP since I posted the first RFR some months ago. The intention of this enhancement is to benefit performance wise from escape analysis even if JVMTI agents request capabilities that allow them to access local variable values. E.g. if you start-up with -agentlib:jdwp=transport=dt_socket,server=y,suspend=n, then escape analysis is disabled right from the beginning, well before a debugger attaches -- if ever one should do so. With the enhancement, escape analysis will remain enabled until and after a debugger attaches. EA based optimizations are reverted just before an agent acquires the reference to an object. In the JBS item you'll find more details. Thanks, Richard. [1] Experimental fix for JDK-8214584 based on JDK-8227745 http://cr.openjdk.java.net/~rrich/webrevs/2019/8214584/experiment_v1.patch
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi David, > Some further queries/concerns: > > src/hotspot/share/runtime/objectMonitor.cpp > > Can you please explain the changes to ObjectMonitor::wait: > > ! _recursions = save // restore the old recursion count > ! + jt->get_and_reset_relock_count_after_wait(); // > increased by the deferred relock count > > what is the "deferred relock count"? I gather it relates to > > "The code was extended to be able to deoptimize objects of a frame that > is not the top frame and to let another thread than the owning thread do > it." Yes, these relate. Currently EA based optimizations are reverted, when a compiled frame is replaced with corresponding interpreter frames. Part of this is relocking objects with eliminated locking. New with the enhancement is that we do this also just before object references are acquired through JVMTI. In this case we deoptimize also the owning compiled frame C and we register deoptimized objects as deferred updates. When control returns to C it gets deoptimized, we notice that objects are already deoptimized (reallocated and relocked), so we don't do it again (relocking twice would be incorrect of course). Deferred updates are copied into the new interpreter frames. Problem: relocking is not possible if the target thread T is waiting on the monitor that needs to be relocked. This happens only with non-local objects with EliminateNestedLocks. Instead relocking is deferred until T owns the monitor again. This is what the piece of code above does. > which I don't like the sound of at all when it comes to ObjectMonitor > state. So I'd like to understand in detail exactly what is going on here > and why. This is a very intrusive change that seems to badly break > encapsulation and impacts future changes to ObjectMonitor that are under > investigation. I would not regard this as breaking encapsulation. Certainly not badly. I've added a property relock_count_after_wait to JavaThread. The property is well encapsulated. Future ObjectMonitor implementations have to deal with recursion too. They are free in choosing a way to do that as long as that property is taken into account. This is hardly a limitation. Note also that the property is a straight forward extension of the existing concept of deferred local updates. It is embedded into the structure holding them. So not even the footprint of a JavaThread is enlarged if no deferred updates are generated. > --- > > src/hotspot/share/runtime/thread.cpp > > Can you please explain why JavaThread::wait_for_object_deoptimization > has to be handcrafted in this way rather than using proper transitions. > I wrote wait_for_object_deoptimization taking JavaThread::java_suspend_self_with_safepoint_check as template. So in short: for the same reasons :) Threads reach both methods as part of thread state transitions, therefore special handling is required to change thread state on top of ongoing transitions. > We got rid of "deopt suspend" some time ago and it is disturbing to see > it being added back (effectively). This seems like it may be something > that handshakes could be used for. Deopt suspend used to be something rather different with a similar name[1]. It is not being added back. I'm actually duplicating the existing external suspend mechanism, because a thread can be suspended at most once. And hey, and don't like that either! But it seems not unlikely that the duplicate can be removed together with the original and the new type of handshakes that will be used for thread suspend can be used for object deoptimization too. See today's discussion in JDK-8227745 [2]. Thanks, Richard. [1] Deopt suspend was something like an async. handshake for architectures with register windows, where patching the return pc for deoptimization of a compiled frame was racy if the owner thread was in native code. Instead a "deopt" suspend flag was set on which the thread patched its own frame upon return from native. So no thread was suspended. It got its name only from the name of the flags. [2] Discussion about using handshakes to sync. with the target thread: https://bugs.openjdk.java.net/browse/JDK-8227745?focusedCommentId=14306727&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14306727 -Original Message- From: David Holmes Sent: Freitag, 13. Dezember 2019 00:56 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, Some further queries/concerns: src/hotspot/share/
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi David, Vladimir, The tests are very targeted and customized towards the issues they solve. IMHO they should be run in the configuration they are tailored for, but as I said, I'm ok with removing the tiered options/conditions. The enhancement should be covered also by existing JVMTI, JDI, JDWP tests, assuming they are also executed with Xcomp. If running the tests with Graal as C2 replacement you'll get failures, because the JVMCI compiler does not provide the debug info required at runtime (see compiledVFrame::not_global_escape_in_scope() and compiledVFrame::arg_escape). Still it would be possible to change the tests to expect these failures when executed with Graal. Perhaps I should do this? Thanks, Richard. -Original Message- From: David Holmes Sent: Freitag, 13. Dezember 2019 02:53 To: Vladimir Kozlov ; Reingruber, Richard ; hotspot-runtime-...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; serviceability-dev@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents On 13/12/2019 10:56 am, Vladimir Kozlov wrote: > Yes, David > > You are correct these changes touch all part of VM and may affect Graal > (which also has EA) too. > Changes should be tested in all our modes: tiered, C1 only, Graal, > Interpreter. And I realized that I only ran tier3-graal testing so I > submitted the rest of Graal's tiers now. > > I had assumed that our current testing (I ran all from tier1 to tier8) > should exercise all paths in VM these changes touch. But I may be wrong > and it is correct to ask author to add testing in all VM modes to make > sure new code in VM's runtime and JVMTI is tested. It may be that our existing JVM TI tests will exercise this adequately and that the new tests are more "whitebox" testing than general functional tests. But it is not obvious to me that we do have the coverage we need. Cheers, David > I do like to keep what current test is doing with C2. May be add an > other test for other modes or modify current one to enable to run it in > other modes. > > Thanks, > Vladimir > > On 12/12/19 3:32 PM, David Holmes wrote: >> On 13/12/2019 9:02 am, Reingruber, Richard wrote: >>> Hello Vladimir, >>> >>> thanks for having a look. >>> >>> > Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to >>> skip >>> > test from running in Interpreter mode too. >>> >>> Done. >>> >>> > You don't need vm.opt.TieredCompilation != true in @requires >>> because you >>> > specified -XX:-TieredCompilation in @run command. >>> >>> Ok. >>> >>> > The test is specifically written for C2 only (not for C1 or >>> Graal) to >>> > verify its Escape Analysis optimization. >>> > I did not look in great details into test's code but its >>> analysis may be >>> > affected if C1 compiler is also used. >>> > >>> > Richard may clarify this. >>> >>> The test cases aim to get their testmethod 'dontinline_testMethod' >>> compiled by C2. If they get C1 >>> compiled before doesn't matter all that much. I've got a slight >>> preference to disabled tiered >>> compilation for simplicity. >> >> My concern - perhaps unfounded - is that this seems to be being tested >> only in a pure C2 environment when the actual changes will have to >> operate correctly in a tiered environment (and JVMCI). >> >> Thanks, >> David >> >>> Thanks, Richard. >>> >>> -Original Message- >>> From: Vladimir Kozlov >>> Sent: Donnerstag, 12. Dezember 2019 19:20 >>> To: David Holmes ; >>> hotspot-runtime-...@openjdk.java.net; >>> hotspot-compiler-...@openjdk.java.net; >>> serviceability-dev@openjdk.java.net; Reingruber, Richard >>> >>> Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better >>> Performance in the Presence of JVMTI Agents >>> >>> Hi David, >>> >>> Tiered is disabled because we don't want to see compilations and outputs >>> from C1 compiler which does not have EA. >>> >>> The test is specifically written for C2 only (not for C1 or Graal) to >>> verify its Escape Analysis optimization. >>> I did not look in great details into test's code but its analysis may be >>> affected if C1 compiler is also used. >>> >>> Richard may clarify this. >>> >>> than
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
On 13/12/2019 10:56 am, Vladimir Kozlov wrote: Yes, David You are correct these changes touch all part of VM and may affect Graal (which also has EA) too. Changes should be tested in all our modes: tiered, C1 only, Graal, Interpreter. And I realized that I only ran tier3-graal testing so I submitted the rest of Graal's tiers now. I had assumed that our current testing (I ran all from tier1 to tier8) should exercise all paths in VM these changes touch. But I may be wrong and it is correct to ask author to add testing in all VM modes to make sure new code in VM's runtime and JVMTI is tested. It may be that our existing JVM TI tests will exercise this adequately and that the new tests are more "whitebox" testing than general functional tests. But it is not obvious to me that we do have the coverage we need. Cheers, David I do like to keep what current test is doing with C2. May be add an other test for other modes or modify current one to enable to run it in other modes. Thanks, Vladimir On 12/12/19 3:32 PM, David Holmes wrote: On 13/12/2019 9:02 am, Reingruber, Richard wrote: Hello Vladimir, thanks for having a look. > Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip > test from running in Interpreter mode too. Done. > You don't need vm.opt.TieredCompilation != true in @requires because you > specified -XX:-TieredCompilation in @run command. Ok. > The test is specifically written for C2 only (not for C1 or Graal) to > verify its Escape Analysis optimization. > I did not look in great details into test's code but its analysis may be > affected if C1 compiler is also used. > > Richard may clarify this. The test cases aim to get their testmethod 'dontinline_testMethod' compiled by C2. If they get C1 compiled before doesn't matter all that much. I've got a slight preference to disabled tiered compilation for simplicity. My concern - perhaps unfounded - is that this seems to be being tested only in a pure C2 environment when the actual changes will have to operate correctly in a tiered environment (and JVMCI). Thanks, David Thanks, Richard. -Original Message- From: Vladimir Kozlov Sent: Donnerstag, 12. Dezember 2019 19:20 To: David Holmes ; hotspot-runtime-...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; serviceability-dev@openjdk.java.net; Reingruber, Richard Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi David, Tiered is disabled because we don't want to see compilations and outputs from C1 compiler which does not have EA. The test is specifically written for C2 only (not for C1 or Graal) to verify its Escape Analysis optimization. I did not look in great details into test's code but its analysis may be affected if C1 compiler is also used. Richard may clarify this. thanks, Vladimir On 12/11/19 1:04 PM, David Holmes wrote: On 12/12/2019 5:21 am, Vladimir Kozlov wrote: I will do full review later. I want to comment about test command line. You don't need vm.opt.TieredCompilation != true in @requires because you specified -XX:-TieredCompilation in @run command. And per my comment this should be being tested with tiered as well. David Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip test from running in Interpreter mode too. Thanks, Vladimir On 12/11/19 7:07 AM, Reingruber, Richard wrote: Hi David, > Most of the details here are in areas I can comment on in detail, but I > did take an initial general look at things. Thanks for taking the time! > The only thing that jumped out at me is that I think the > DeoptimizeObjectsALotThread should be a hidden thread. > > + bool is_hidden_from_external_view() const { return true; } Yes, it should. Will add the method like above. > Also I don't see any testing of the DeoptimizeObjectsALotThread. Without > active testing this will just bit-rot. DeoptimizeObjectsALot is meant for stress testing with a larger workload. I will add a minimal test to keep it fresh. > Also on the tests I don't understand your @requires clause: > > @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & > (vm.opt.TieredCompilation != true)) > > This seems to require that TieredCompilation is disabled, but tiered is > our normal mode of operation. ?? > I removed the clause. I guess I wanted to target the tests towards the code they are supposed to test, and it's easier to analyze failures w/o tiered compilation and with just one compiler thread. Additionally I will make use of compiler.whitebox.CompilerWhiteBoxTest.THRESHOLD in the tests. Thanks, Richard. -Original Mes
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Yes, David You are correct these changes touch all part of VM and may affect Graal (which also has EA) too. Changes should be tested in all our modes: tiered, C1 only, Graal, Interpreter. And I realized that I only ran tier3-graal testing so I submitted the rest of Graal's tiers now. I had assumed that our current testing (I ran all from tier1 to tier8) should exercise all paths in VM these changes touch. But I may be wrong and it is correct to ask author to add testing in all VM modes to make sure new code in VM's runtime and JVMTI is tested. I do like to keep what current test is doing with C2. May be add an other test for other modes or modify current one to enable to run it in other modes. Thanks, Vladimir On 12/12/19 3:32 PM, David Holmes wrote: On 13/12/2019 9:02 am, Reingruber, Richard wrote: Hello Vladimir, thanks for having a look. > Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip > test from running in Interpreter mode too. Done. > You don't need vm.opt.TieredCompilation != true in @requires because you > specified -XX:-TieredCompilation in @run command. Ok. > The test is specifically written for C2 only (not for C1 or Graal) to > verify its Escape Analysis optimization. > I did not look in great details into test's code but its analysis may be > affected if C1 compiler is also used. > > Richard may clarify this. The test cases aim to get their testmethod 'dontinline_testMethod' compiled by C2. If they get C1 compiled before doesn't matter all that much. I've got a slight preference to disabled tiered compilation for simplicity. My concern - perhaps unfounded - is that this seems to be being tested only in a pure C2 environment when the actual changes will have to operate correctly in a tiered environment (and JVMCI). Thanks, David Thanks, Richard. -Original Message- From: Vladimir Kozlov Sent: Donnerstag, 12. Dezember 2019 19:20 To: David Holmes ; hotspot-runtime-...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; serviceability-dev@openjdk.java.net; Reingruber, Richard Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi David, Tiered is disabled because we don't want to see compilations and outputs from C1 compiler which does not have EA. The test is specifically written for C2 only (not for C1 or Graal) to verify its Escape Analysis optimization. I did not look in great details into test's code but its analysis may be affected if C1 compiler is also used. Richard may clarify this. thanks, Vladimir On 12/11/19 1:04 PM, David Holmes wrote: On 12/12/2019 5:21 am, Vladimir Kozlov wrote: I will do full review later. I want to comment about test command line. You don't need vm.opt.TieredCompilation != true in @requires because you specified -XX:-TieredCompilation in @run command. And per my comment this should be being tested with tiered as well. David Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip test from running in Interpreter mode too. Thanks, Vladimir On 12/11/19 7:07 AM, Reingruber, Richard wrote: Hi David, > Most of the details here are in areas I can comment on in detail, but I > did take an initial general look at things. Thanks for taking the time! > The only thing that jumped out at me is that I think the > DeoptimizeObjectsALotThread should be a hidden thread. > > + bool is_hidden_from_external_view() const { return true; } Yes, it should. Will add the method like above. > Also I don't see any testing of the DeoptimizeObjectsALotThread. Without > active testing this will just bit-rot. DeoptimizeObjectsALot is meant for stress testing with a larger workload. I will add a minimal test to keep it fresh. > Also on the tests I don't understand your @requires clause: > > @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & > (vm.opt.TieredCompilation != true)) > > This seems to require that TieredCompilation is disabled, but tiered is > our normal mode of operation. ?? > I removed the clause. I guess I wanted to target the tests towards the code they are supposed to test, and it's easier to analyze failures w/o tiered compilation and with just one compiler thread. Additionally I will make use of compiler.whitebox.CompilerWhiteBoxTest.THRESHOLD in the tests. Thanks, Richard. -Original Message- From: David Holmes Sent: Mittwoch, 11. Dezember 2019 08:03 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI A
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, Some further queries/concerns: src/hotspot/share/runtime/objectMonitor.cpp Can you please explain the changes to ObjectMonitor::wait: ! _recursions = save // restore the old recursion count ! + jt->get_and_reset_relock_count_after_wait(); // increased by the deferred relock count what is the "deferred relock count"? I gather it relates to "The code was extended to be able to deoptimize objects of a frame that is not the top frame and to let another thread than the owning thread do it." which I don't like the sound of at all when it comes to ObjectMonitor state. So I'd like to understand in detail exactly what is going on here and why. This is a very intrusive change that seems to badly break encapsulation and impacts future changes to ObjectMonitor that are under investigation. --- src/hotspot/share/runtime/thread.cpp Can you please explain why JavaThread::wait_for_object_deoptimization has to be handcrafted in this way rather than using proper transitions. We got rid of "deopt suspend" some time ago and it is disturbing to see it being added back (effectively). This seems like it may be something that handshakes could be used for. Thanks, David - On 12/12/2019 7:02 am, David Holmes wrote: On 12/12/2019 1:07 am, Reingruber, Richard wrote: Hi David, > Most of the details here are in areas I can comment on in detail, but I > did take an initial general look at things. Thanks for taking the time! Apologies the above should read: "Most of the details here are in areas I *can't* comment on in detail ..." David > The only thing that jumped out at me is that I think the > DeoptimizeObjectsALotThread should be a hidden thread. > > + bool is_hidden_from_external_view() const { return true; } Yes, it should. Will add the method like above. > Also I don't see any testing of the DeoptimizeObjectsALotThread. Without > active testing this will just bit-rot. DeoptimizeObjectsALot is meant for stress testing with a larger workload. I will add a minimal test to keep it fresh. > Also on the tests I don't understand your @requires clause: > > @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & > (vm.opt.TieredCompilation != true)) > > This seems to require that TieredCompilation is disabled, but tiered is > our normal mode of operation. ?? > I removed the clause. I guess I wanted to target the tests towards the code they are supposed to test, and it's easier to analyze failures w/o tiered compilation and with just one compiler thread. Additionally I will make use of compiler.whitebox.CompilerWhiteBoxTest.THRESHOLD in the tests. Thanks, Richard. -Original Message- From: David Holmes Sent: Mittwoch, 11. Dezember 2019 08:03 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, On 11/12/2019 7:45 am, Reingruber, Richard wrote: Hi, I would like to get reviews please for http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/ Corresponding RFE: https://bugs.openjdk.java.net/browse/JDK-8227745 Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915 And potentially https://bugs.openjdk.java.net/browse/JDK-8214584 [1] Vladimir Kozlov kindly put webrev.3 through tier1-8 testing without issues (thanks!). In addition the change is being tested at SAP since I posted the first RFR some months ago. The intention of this enhancement is to benefit performance wise from escape analysis even if JVMTI agents request capabilities that allow them to access local variable values. E.g. if you start-up with -agentlib:jdwp=transport=dt_socket,server=y,suspend=n, then escape analysis is disabled right from the beginning, well before a debugger attaches -- if ever one should do so. With the enhancement, escape analysis will remain enabled until and after a debugger attaches. EA based optimizations are reverted just before an agent acquires the reference to an object. In the JBS item you'll find more details. Most of the details here are in areas I can comment on in detail, but I did take an initial general look at things. The only thing that jumped out at me is that I think the DeoptimizeObjectsALotThread should be a hidden thread. + bool is_hidden_from_external_view() const { return true; } Also I don't see any testing of the DeoptimizeObjectsALotThread. Without active testing this will just bit-rot. Also on the tests I don't understand your @requires clause: @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & (vm.opt.TieredCompilation != true)) This seems to
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
On 13/12/2019 9:02 am, Reingruber, Richard wrote: Hello Vladimir, thanks for having a look. > Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip > test from running in Interpreter mode too. Done. > You don't need vm.opt.TieredCompilation != true in @requires because you > specified -XX:-TieredCompilation in @run command. Ok. > The test is specifically written for C2 only (not for C1 or Graal) to > verify its Escape Analysis optimization. > I did not look in great details into test's code but its analysis may be > affected if C1 compiler is also used. > > Richard may clarify this. The test cases aim to get their testmethod 'dontinline_testMethod' compiled by C2. If they get C1 compiled before doesn't matter all that much. I've got a slight preference to disabled tiered compilation for simplicity. My concern - perhaps unfounded - is that this seems to be being tested only in a pure C2 environment when the actual changes will have to operate correctly in a tiered environment (and JVMCI). Thanks, David Thanks, Richard. -Original Message- From: Vladimir Kozlov Sent: Donnerstag, 12. Dezember 2019 19:20 To: David Holmes ; hotspot-runtime-...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; serviceability-dev@openjdk.java.net; Reingruber, Richard Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi David, Tiered is disabled because we don't want to see compilations and outputs from C1 compiler which does not have EA. The test is specifically written for C2 only (not for C1 or Graal) to verify its Escape Analysis optimization. I did not look in great details into test's code but its analysis may be affected if C1 compiler is also used. Richard may clarify this. thanks, Vladimir On 12/11/19 1:04 PM, David Holmes wrote: On 12/12/2019 5:21 am, Vladimir Kozlov wrote: I will do full review later. I want to comment about test command line. You don't need vm.opt.TieredCompilation != true in @requires because you specified -XX:-TieredCompilation in @run command. And per my comment this should be being tested with tiered as well. David Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip test from running in Interpreter mode too. Thanks, Vladimir On 12/11/19 7:07 AM, Reingruber, Richard wrote: Hi David, > Most of the details here are in areas I can comment on in detail, but I > did take an initial general look at things. Thanks for taking the time! > The only thing that jumped out at me is that I think the > DeoptimizeObjectsALotThread should be a hidden thread. > > + bool is_hidden_from_external_view() const { return true; } Yes, it should. Will add the method like above. > Also I don't see any testing of the DeoptimizeObjectsALotThread. Without > active testing this will just bit-rot. DeoptimizeObjectsALot is meant for stress testing with a larger workload. I will add a minimal test to keep it fresh. > Also on the tests I don't understand your @requires clause: > > @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & > (vm.opt.TieredCompilation != true)) > > This seems to require that TieredCompilation is disabled, but tiered is > our normal mode of operation. ?? > I removed the clause. I guess I wanted to target the tests towards the code they are supposed to test, and it's easier to analyze failures w/o tiered compilation and with just one compiler thread. Additionally I will make use of compiler.whitebox.CompilerWhiteBoxTest.THRESHOLD in the tests. Thanks, Richard. -Original Message- From: David Holmes Sent: Mittwoch, 11. Dezember 2019 08:03 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, On 11/12/2019 7:45 am, Reingruber, Richard wrote: Hi, I would like to get reviews please for http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/ Corresponding RFE: https://bugs.openjdk.java.net/browse/JDK-8227745 Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915 And potentially https://bugs.openjdk.java.net/browse/JDK-8214584 [1] Vladimir Kozlov kindly put webrev.3 through tier1-8 testing without issues (thanks!). In addition the change is being tested at SAP since I posted the first RFR some months ago. The intention of this enhancement is to benefit performance wise from escape analysis even if JVMTI agents request capabilities that allow them to access local variable values. E.g. if you start-up with -agentlib:jdwp=transport=dt_
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hello Vladimir, thanks for having a look. > Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip > test from running in Interpreter mode too. Done. > You don't need vm.opt.TieredCompilation != true in @requires because you > specified -XX:-TieredCompilation in @run command. Ok. > The test is specifically written for C2 only (not for C1 or Graal) to > verify its Escape Analysis optimization. > I did not look in great details into test's code but its analysis may be > affected if C1 compiler is also used. > > Richard may clarify this. The test cases aim to get their testmethod 'dontinline_testMethod' compiled by C2. If they get C1 compiled before doesn't matter all that much. I've got a slight preference to disabled tiered compilation for simplicity. Thanks, Richard. -Original Message- From: Vladimir Kozlov Sent: Donnerstag, 12. Dezember 2019 19:20 To: David Holmes ; hotspot-runtime-...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; serviceability-dev@openjdk.java.net; Reingruber, Richard Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi David, Tiered is disabled because we don't want to see compilations and outputs from C1 compiler which does not have EA. The test is specifically written for C2 only (not for C1 or Graal) to verify its Escape Analysis optimization. I did not look in great details into test's code but its analysis may be affected if C1 compiler is also used. Richard may clarify this. thanks, Vladimir On 12/11/19 1:04 PM, David Holmes wrote: > On 12/12/2019 5:21 am, Vladimir Kozlov wrote: >> I will do full review later. I want to comment about test command line. >> >> You don't need vm.opt.TieredCompilation != true in @requires because >> you specified -XX:-TieredCompilation in @run command. > > And per my comment this should be being tested with tiered as well. > > David > >> Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip >> test from running in Interpreter mode too. >> >> Thanks, >> Vladimir >> >> On 12/11/19 7:07 AM, Reingruber, Richard wrote: >>> Hi David, >>> >>> > Most of the details here are in areas I can comment on in >>> detail, but I >>> > did take an initial general look at things. >>> >>> Thanks for taking the time! >>> >>> > The only thing that jumped out at me is that I think the >>> > DeoptimizeObjectsALotThread should be a hidden thread. >>> > >>> > + bool is_hidden_from_external_view() const { return true; } >>> >>> Yes, it should. Will add the method like above. >>> >>> > Also I don't see any testing of the DeoptimizeObjectsALotThread. >>> Without >>> > active testing this will just bit-rot. >>> >>> DeoptimizeObjectsALot is meant for stress testing with a larger >>> workload. I will add a minimal test >>> to keep it fresh. >>> >>> > Also on the tests I don't understand your @requires clause: >>> > >>> > @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & >>> > (vm.opt.TieredCompilation != true)) >>> > >>> > This seems to require that TieredCompilation is disabled, but >>> tiered is >>> > our normal mode of operation. ?? >>> > >>> >>> I removed the clause. I guess I wanted to target the tests towards >>> the code they are supposed to >>> test, and it's easier to analyze failures w/o tiered compilation and >>> with just one compiler thread. >>> >>> Additionally I will make use of >>> compiler.whitebox.CompilerWhiteBoxTest.THRESHOLD in the tests. >>> >>> Thanks, >>> Richard. >>> >>> -Original Message- >>> From: David Holmes >>> Sent: Mittwoch, 11. Dezember 2019 08:03 >>> To: Reingruber, Richard ; >>> serviceability-dev@openjdk.java.net; >>> hotspot-compiler-...@openjdk.java.net; >>> hotspot-runtime-...@openjdk.java.net >>> Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better >>> Performance in the Presence of JVMTI Agents >>> >>> Hi Richard, >>> >>> On 11/12/2019 7:45 am, Reingruber, Richard wrote: >>>> Hi, >>>> >>>> I would like to get reviews please for >>>> >>>> http://cr.openjdk
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi David, Tiered is disabled because we don't want to see compilations and outputs from C1 compiler which does not have EA. The test is specifically written for C2 only (not for C1 or Graal) to verify its Escape Analysis optimization. I did not look in great details into test's code but its analysis may be affected if C1 compiler is also used. Richard may clarify this. thanks, Vladimir On 12/11/19 1:04 PM, David Holmes wrote: On 12/12/2019 5:21 am, Vladimir Kozlov wrote: I will do full review later. I want to comment about test command line. You don't need vm.opt.TieredCompilation != true in @requires because you specified -XX:-TieredCompilation in @run command. And per my comment this should be being tested with tiered as well. David Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip test from running in Interpreter mode too. Thanks, Vladimir On 12/11/19 7:07 AM, Reingruber, Richard wrote: Hi David, > Most of the details here are in areas I can comment on in detail, but I > did take an initial general look at things. Thanks for taking the time! > The only thing that jumped out at me is that I think the > DeoptimizeObjectsALotThread should be a hidden thread. > > + bool is_hidden_from_external_view() const { return true; } Yes, it should. Will add the method like above. > Also I don't see any testing of the DeoptimizeObjectsALotThread. Without > active testing this will just bit-rot. DeoptimizeObjectsALot is meant for stress testing with a larger workload. I will add a minimal test to keep it fresh. > Also on the tests I don't understand your @requires clause: > > @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & > (vm.opt.TieredCompilation != true)) > > This seems to require that TieredCompilation is disabled, but tiered is > our normal mode of operation. ?? > I removed the clause. I guess I wanted to target the tests towards the code they are supposed to test, and it's easier to analyze failures w/o tiered compilation and with just one compiler thread. Additionally I will make use of compiler.whitebox.CompilerWhiteBoxTest.THRESHOLD in the tests. Thanks, Richard. -Original Message- From: David Holmes Sent: Mittwoch, 11. Dezember 2019 08:03 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, On 11/12/2019 7:45 am, Reingruber, Richard wrote: Hi, I would like to get reviews please for http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/ Corresponding RFE: https://bugs.openjdk.java.net/browse/JDK-8227745 Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915 And potentially https://bugs.openjdk.java.net/browse/JDK-8214584 [1] Vladimir Kozlov kindly put webrev.3 through tier1-8 testing without issues (thanks!). In addition the change is being tested at SAP since I posted the first RFR some months ago. The intention of this enhancement is to benefit performance wise from escape analysis even if JVMTI agents request capabilities that allow them to access local variable values. E.g. if you start-up with -agentlib:jdwp=transport=dt_socket,server=y,suspend=n, then escape analysis is disabled right from the beginning, well before a debugger attaches -- if ever one should do so. With the enhancement, escape analysis will remain enabled until and after a debugger attaches. EA based optimizations are reverted just before an agent acquires the reference to an object. In the JBS item you'll find more details. Most of the details here are in areas I can comment on in detail, but I did take an initial general look at things. The only thing that jumped out at me is that I think the DeoptimizeObjectsALotThread should be a hidden thread. + bool is_hidden_from_external_view() const { return true; } Also I don't see any testing of the DeoptimizeObjectsALotThread. Without active testing this will just bit-rot. Also on the tests I don't understand your @requires clause: @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & (vm.opt.TieredCompilation != true)) This seems to require that TieredCompilation is disabled, but tiered is our normal mode of operation. ?? Thanks, David Thanks, Richard. [1] Experimental fix for JDK-8214584 based on JDK-8227745 http://cr.openjdk.java.net/~rrich/webrevs/2019/8214584/experiment_v1.patch
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
On 12/12/2019 1:07 am, Reingruber, Richard wrote: Hi David, > Most of the details here are in areas I can comment on in detail, but I > did take an initial general look at things. Thanks for taking the time! Apologies the above should read: "Most of the details here are in areas I *can't* comment on in detail ..." David > The only thing that jumped out at me is that I think the > DeoptimizeObjectsALotThread should be a hidden thread. > > + bool is_hidden_from_external_view() const { return true; } Yes, it should. Will add the method like above. > Also I don't see any testing of the DeoptimizeObjectsALotThread. Without > active testing this will just bit-rot. DeoptimizeObjectsALot is meant for stress testing with a larger workload. I will add a minimal test to keep it fresh. > Also on the tests I don't understand your @requires clause: > > @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & > (vm.opt.TieredCompilation != true)) > > This seems to require that TieredCompilation is disabled, but tiered is > our normal mode of operation. ?? > I removed the clause. I guess I wanted to target the tests towards the code they are supposed to test, and it's easier to analyze failures w/o tiered compilation and with just one compiler thread. Additionally I will make use of compiler.whitebox.CompilerWhiteBoxTest.THRESHOLD in the tests. Thanks, Richard. -Original Message- From: David Holmes Sent: Mittwoch, 11. Dezember 2019 08:03 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, On 11/12/2019 7:45 am, Reingruber, Richard wrote: Hi, I would like to get reviews please for http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/ Corresponding RFE: https://bugs.openjdk.java.net/browse/JDK-8227745 Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915 And potentially https://bugs.openjdk.java.net/browse/JDK-8214584 [1] Vladimir Kozlov kindly put webrev.3 through tier1-8 testing without issues (thanks!). In addition the change is being tested at SAP since I posted the first RFR some months ago. The intention of this enhancement is to benefit performance wise from escape analysis even if JVMTI agents request capabilities that allow them to access local variable values. E.g. if you start-up with -agentlib:jdwp=transport=dt_socket,server=y,suspend=n, then escape analysis is disabled right from the beginning, well before a debugger attaches -- if ever one should do so. With the enhancement, escape analysis will remain enabled until and after a debugger attaches. EA based optimizations are reverted just before an agent acquires the reference to an object. In the JBS item you'll find more details. Most of the details here are in areas I can comment on in detail, but I did take an initial general look at things. The only thing that jumped out at me is that I think the DeoptimizeObjectsALotThread should be a hidden thread. + bool is_hidden_from_external_view() const { return true; } Also I don't see any testing of the DeoptimizeObjectsALotThread. Without active testing this will just bit-rot. Also on the tests I don't understand your @requires clause: @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & (vm.opt.TieredCompilation != true)) This seems to require that TieredCompilation is disabled, but tiered is our normal mode of operation. ?? Thanks, David Thanks, Richard. [1] Experimental fix for JDK-8214584 based on JDK-8227745 http://cr.openjdk.java.net/~rrich/webrevs/2019/8214584/experiment_v1.patch
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi David, > Most of the details here are in areas I can comment on in detail, but I > did take an initial general look at things. Thanks for taking the time! > The only thing that jumped out at me is that I think the > DeoptimizeObjectsALotThread should be a hidden thread. > > + bool is_hidden_from_external_view() const { return true; } Yes, it should. Will add the method like above. > Also I don't see any testing of the DeoptimizeObjectsALotThread. Without > active testing this will just bit-rot. DeoptimizeObjectsALot is meant for stress testing with a larger workload. I will add a minimal test to keep it fresh. > Also on the tests I don't understand your @requires clause: > > @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & > (vm.opt.TieredCompilation != true)) > > This seems to require that TieredCompilation is disabled, but tiered is > our normal mode of operation. ?? > I removed the clause. I guess I wanted to target the tests towards the code they are supposed to test, and it's easier to analyze failures w/o tiered compilation and with just one compiler thread. Additionally I will make use of compiler.whitebox.CompilerWhiteBoxTest.THRESHOLD in the tests. Thanks, Richard. -Original Message- From: David Holmes Sent: Mittwoch, 11. Dezember 2019 08:03 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, On 11/12/2019 7:45 am, Reingruber, Richard wrote: > Hi, > > I would like to get reviews please for > > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/ > > Corresponding RFE: > https://bugs.openjdk.java.net/browse/JDK-8227745 > > Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915 > And potentially https://bugs.openjdk.java.net/browse/JDK-8214584 [1] > > Vladimir Kozlov kindly put webrev.3 through tier1-8 testing without issues > (thanks!). In addition the > change is being tested at SAP since I posted the first RFR some months ago. > > The intention of this enhancement is to benefit performance wise from escape > analysis even if JVMTI > agents request capabilities that allow them to access local variable values. > E.g. if you start-up > with -agentlib:jdwp=transport=dt_socket,server=y,suspend=n, then escape > analysis is disabled right > from the beginning, well before a debugger attaches -- if ever one should do > so. With the > enhancement, escape analysis will remain enabled until and after a debugger > attaches. EA based > optimizations are reverted just before an agent acquires the reference to an > object. In the JBS item > you'll find more details. Most of the details here are in areas I can comment on in detail, but I did take an initial general look at things. The only thing that jumped out at me is that I think the DeoptimizeObjectsALotThread should be a hidden thread. + bool is_hidden_from_external_view() const { return true; } Also I don't see any testing of the DeoptimizeObjectsALotThread. Without active testing this will just bit-rot. Also on the tests I don't understand your @requires clause: @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & (vm.opt.TieredCompilation != true)) This seems to require that TieredCompilation is disabled, but tiered is our normal mode of operation. ?? Thanks, David > Thanks, > Richard. > > [1] Experimental fix for JDK-8214584 based on JDK-8227745 > > http://cr.openjdk.java.net/~rrich/webrevs/2019/8214584/experiment_v1.patch >
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, On 11/12/2019 7:45 am, Reingruber, Richard wrote: Hi, I would like to get reviews please for http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/ Corresponding RFE: https://bugs.openjdk.java.net/browse/JDK-8227745 Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915 And potentially https://bugs.openjdk.java.net/browse/JDK-8214584 [1] Vladimir Kozlov kindly put webrev.3 through tier1-8 testing without issues (thanks!). In addition the change is being tested at SAP since I posted the first RFR some months ago. The intention of this enhancement is to benefit performance wise from escape analysis even if JVMTI agents request capabilities that allow them to access local variable values. E.g. if you start-up with -agentlib:jdwp=transport=dt_socket,server=y,suspend=n, then escape analysis is disabled right from the beginning, well before a debugger attaches -- if ever one should do so. With the enhancement, escape analysis will remain enabled until and after a debugger attaches. EA based optimizations are reverted just before an agent acquires the reference to an object. In the JBS item you'll find more details. Most of the details here are in areas I can comment on in detail, but I did take an initial general look at things. The only thing that jumped out at me is that I think the DeoptimizeObjectsALotThread should be a hidden thread. + bool is_hidden_from_external_view() const { return true; } Also I don't see any testing of the DeoptimizeObjectsALotThread. Without active testing this will just bit-rot. Also on the tests I don't understand your @requires clause: @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled & (vm.opt.TieredCompilation != true)) This seems to require that TieredCompilation is disabled, but tiered is our normal mode of operation. ?? Thanks, David Thanks, Richard. [1] Experimental fix for JDK-8214584 based on JDK-8227745 http://cr.openjdk.java.net/~rrich/webrevs/2019/8214584/experiment_v1.patch