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

[Reingruber, Richard]

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 <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.