RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents

[Lindenmaier, Goetz]

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.reingruber at sap.com>
> Sent: Thursday, August 27, 2020 10:33 PM
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; serviceability-
> dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; hotspot-
> runtime-dev at 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.lindenmaier at sap.com>
> Sent: Samstag, 22. August 2020 07:46
> To: Reingruber, Richard <richard.reingruber at sap.com>; serviceability-
> dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; hotspot-
> runtime-dev at 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.