Thanks a lot! Richard.
-----Original Message----- From: Lindenmaier, Goetz <goetz.lindenma...@sap.com> Sent: Freitag, 28. August 2020 08:38 To: Reingruber, Richard <richard.reingru...@sap.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 the new webrev. The small improvements are fine, too. Reviewed from my side. Best regards, Goetz. > -----Original Message----- > From: Reingruber, Richard <richard.reingru...@sap.com> > Sent: Thursday, August 27, 2020 10:33 PM > To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; 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 <goetz.lindenma...@sap.com> > Sent: Samstag, 22. August 2020 07:46 > To: Reingruber, Richard <richard.reingru...@sap.com>; 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.
