8282024: add EscapeAnalysis statistics under PrintOptoStatistics #8019

aamarsh · 2022-03-29T17:31:29Z

Escape Analysis and Scalar Replacement statistics were added when the -XX:+PrintOptoStatistics flag is set. All code is placed in #ifndef Product block, so this code is only run when creating a debug build. Using renaissance benchmark I ran a few tests to confirm that numbers were printing correctly. Below is an example run:

No escape = 263, Arg escape = 87, Global escape = 1628
Objects scalar replaced = 193, Monitor objects removed = 32, GC barriers removed = 38, Memory barriers removed = 225

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8282024: add EscapeAnalysis statistics under PrintOptoStatistics

Reviewers

Xin Liu (@navyxliu - Committer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8019/head:pull/8019
$ git checkout pull/8019

Update a local copy of the PR:
$ git checkout pull/8019
$ git pull https://git.openjdk.java.net/jdk pull/8019/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8019

View PR using the GUI difftool:
$ git pr show -t 8019

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8019.diff

bridgekeeper · 2022-03-29T17:31:38Z

👋 Welcome back aamarsh! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-03-29T17:33:54Z

@aamarsh The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

src/hotspot/share/opto/macro.cpp

src/hotspot/share/opto/escape.cpp

mlbridge · 2022-03-31T21:19:11Z

Webrevs

navyxliu · 2022-04-01T01:56:20Z

hi, @aamarsh ,
Generally speaking, the scalar replaced objects should be the subset of NonEscape objects.

In your case, NoEscape/Objects scalar replaced is 124/172. One potential explanation is that you encounter many boxing objects. Is this case?

  // Eliminate boxing allocations which are not used
  // regardless scalar replacable status.
  bool boxing_alloc = C->eliminate_boxing() &&
                      tklass->klass()->is_instance_klass()  &&
                      tklass->klass()->as_instance_klass()->is_box_klass();
  if (!alloc->_is_scalar_replaceable && (!boxing_alloc || (res != NULL))) {
    return false;
  }

I see that you add static counters to ConnectionGraph and PhaseMacroExpand. Do we need to use Atomic::add in case many C2 compiler threads run in parallel. if you combine CICompilerCount and -Xcomp, can you see those counters are stable values?

Even we won't completely mess up counters, it may lose values in multi-threaded environment.

PhaseMacroExpand::_objs_scalar_replaced_counter++;

aamarsh · 2022-04-04T22:26:30Z

hi, @navyxliu

Yes, you are correct that scalar replaced objects should be a subset of NonEscape objects. In my implementation I was resetting the NonEscape counter if escape analysis was called multiple times on a ConnectionGraph. Unfortunately I was forgetting to count the objects that had been scalar replaced in between escape analysis calls. I have updated my logic to support this and I updated the provided example. Thank you for catching that!

As for using static counters, the thought of a multi-threaded environment did come up. I looked at other static counters printed in Compile::print_statistics() for examples. None of the static counters that I found took into account a multi-threaded environment. Some of these counters include SharedRuntime::_implicit_null_throws, PhaseCCP::_total_invokes, PhaseCCP::_total_constants, PhaseOutput::total_nop_size, and PhasePeephole::total_peepholes. But adding this logic most definitley does not hurt, so I did use the Atomic functions to load/store/add my static counters.

navyxliu · 2022-04-05T02:02:41Z

hi, @aamarsh ,
I understand that. I guess it wasn't easy to have portal atomic operations. The situation has improved since atomic.hpp is introduced. we can use it as easy as standard c++. All those counters are in debug build, so we won't pay anything in release build. let's leave old counters for future work.

src/hotspot/share/opto/compile.cpp

src/hotspot/share/opto/escape.cpp

src/hotspot/share/opto/macro.cpp

navyxliu

hi,

Thank you for working on this. In generally, I think it looks good. only small suggests left.

src/hotspot/share/opto/compile.cpp

navyxliu · 2022-05-04T05:24:08Z

It seems that you use git reset and push it forcely. That's why you only have one commit displayed in this PR.
Good news is that Skara can still generate revisions.

I think it's easier for people to keep track your changes incrementally. It's totally okay you keep committing to your branch. github will squash them when it merges a PR. please refrain from push by force.

navyxliu

LTGM. I am not a reviewer. We need other reviewers to approve it.

navyxliu · 2022-05-05T19:06:40Z

src/hotspot/share/opto/escape.cpp

+      else if (ptn->escape_state() == PointsToNode::ArgEscape) {
+        _compile->_local_arg_escape_ctr++;
+      }
+      else if (ptn->escape_state() == PointsToNode::GlobalEscape) {


"else if" style is not consistent with others.

NIT: Move "else if" to line 3793.

vnkozlov

I have few comments.

src/hotspot/share/opto/compile.cpp

vnkozlov · 2022-05-10T00:21:45Z

src/hotspot/share/opto/escape.cpp

+#ifndef PRODUCT
+    escape_state_statistics(java_objects_worklist);
+#endif


You can use NOT_PRODUCT() macro for one line which you have a lot in these changes.

NIT: Use NOT_PRODUCT

src/hotspot/share/opto/escape.cpp

vnkozlov · 2022-05-10T00:27:37Z

src/hotspot/share/opto/escape.cpp

+  _compile->_local_no_escape_ctr = 0;
+  _compile->_local_arg_escape_ctr = 0;
+  _compile->_local_global_escape_ctr = 0;


I don't think you need these Compile counters - make them local to update corresponding ConnectionGraph counters after loop. As I said these counters are not accurate anyway. Unless you want to track for which allocation which counter was recorded. Which I think will be over-kill because the most EA done with one iteration.

vnkozlov · 2022-05-10T00:29:30Z

src/hotspot/share/opto/macro.hpp

+    static int _monitor_objects_removed_counter;
+    static int _GC_barriers_removed_counter;
+    static int _memory_barriers_removed_counter;
+    int _local_scalar_replaced;


You don't need _local_scalar_replaced value as I said in an other comment.

src/hotspot/share/opto/escape.cpp

src/hotspot/share/opto/macro.cpp

vnkozlov · 2022-05-20T18:52:52Z

src/hotspot/share/opto/escape.cpp

+void ConnectionGraph::print_statistics() {
+  tty->print("No escape = %d, Arg escape = %d, Global escape = %d", Atomic::load(&_no_escape_counter), Atomic::load(&_arg_escape_counter), Atomic::load(&_global_escape_counter));
+  tty->print(" (EA executed in %7.2f seconds)", Atomic::load(&_time_elapsed) * 0.001);
+  tty->print_cr("   ** EA stats might be slightly off since objects might be double counted due to iterative EA **");


I don't think we need to print this warning. I suggest to put it as comment in this method.

vnkozlov

Thank you for addressing my comment. I have only one left.
I started testing.

vnkozlov

About time statistic. There is flag -XX:+CITime which prints times for all phases in C2 (and C1). For EA it is _t_escapeAnalysis. And I see difference in reported times (0.02 vs 0.059).

Your output:

(EA executed in    0.02 seconds)

CITime:

         Escape Analysis:       0.076 s
           Conn Graph:            0.059 s
           Macro Eliminate:       0.027 s

I simply run fastdebug VM with -Xcomp:

java -XX:+PrintOptoStatistics -XX:+CITime -Xcomp t

vnkozlov · 2022-05-20T19:21:21Z

It is up to you but I would suggest to remove your change to collect time since we have it already with CITime.

navyxliu · 2022-05-20T23:34:25Z

src/hotspot/share/opto/escape.cpp

@@ -3739,6 +3752,31 @@ void ConnectionGraph::dump(GrowableArray<PointsToNode*>& ptnodes_worklist) {
  }
 }

+void ConnectionGraph::print_statistics() {
+  // EA stats might be slightly off since objects might be double counted due to iterative EA


I don't understand. your approach almost worked in last revision. All you need to do is to adjust for the last iteration.

This revision drops it. I don't think it would be "slightly" off, even double counted is optimistic. A java object will be counted repeat if iterEA iterates N times. My concern is the final statistical counters become incomparable.

Why did you drop snapshot approach in 0805514?

@navyxliu, please, explain what do you mean "snapshot approach" and suggest how we should do it.

In previous revision, @aamarsh used 3 data members of Compile. This tuple is a snapshot.

_local_no_escape_ctr _local_arg_escape_ctr _local_global_escape_ctr

ConnectionGraph::escape_state_statistics() initializes them all zeros and categorize Java objects of the connection graph.

here is the framework.

do { EscapeAnalysis(); MacroExpand.eliminate_macro_nodes(); } while (progress?); #ifndef PRODUCT Atomic::add(&ConnectionGraph::_no_escape_counter, _local_no_escape_ctr + total_scalar_replaced); Atomic::add(&ConnectionGraph::_arg_escape_counter, _local_arg_escape_ctr); Atomic::add(&ConnectionGraph::_global_escape_counter, _local_global_escape_ctr); #endif

Both you and @JohnTortugo pointed out that there was a bug in previous revision. We overlook that non-escaped objects are double-counted in last iteration. I think it's amendable.

I call this snapshot approach because it uses the snapshot of last iteration to update global statistical counters. all intermediate snapshots are drop. Allow me to write down @aamarsh 's approach.

The number of Java object JO is from user-program by nature. EscapeAnalysis breaks them down into 3 categories. non-escaped, arg-escaped and global escaped. Without iterative EA, we can report this snapshot to statistical counters.

With iterative EA, a problem arise. Some objects elided in previous iteration of MacroExpansion. if we use last snapshot, we have to add those eliminated java objects. Let's say iterative EA iterates N times in total. E is the number of eliminated java object from 1 to N-1 iterations (exclude the last iteration here).

Since all eliminated objects must be non-escaped, so we add E back to _no_escape_counter. We account for all java objects of this compilation unit.

Atomic::add(&ConnectionGraph::_no_escape_counter, _local_no_escape_ctr + E); Atomic::add(&ConnectionGraph::_arg_escape_counter, _local_arg_escape_ctr); Atomic::add(&ConnectionGraph::_global_escape_counter, _local_global_escape_ctr);

In this way, We keep track of all java objects of this CU for iterative EA. I think it's accurate.

JO = _local_no_escape_ctr + E + _local_arg_escape_ctr + _local_global_escape_ctr

I suggest to hoist the variable PhaseMacroExpand mexp out of loop and make it stateful to track E.

Got it. Thank for explaining - you simply used data from last iteration.
I think you can do similar with much less complexity if you used data from first iteration and current code change:

void ConnectionGraph::escape_state_statistics(GrowableArray<JavaObjectNode*>& java_objects_worklist) { if (!PrintOptoStatistics || (_invocation > 0)) { // Collect data only for first invocation return; } for (int next = 0; next < java_objects_worklist.length(); ++next) {

You need to revert escape_state_statistics back to non static for this of cause.

so you use the snapshot of first iteration. We assume Iterative EA can elide some non-escaped objects but can't change any object's category. I think it is correct and indeed much simpler!

That was a great idea. Thank you all. I'll work with @aamarsh to do the changes.

navyxliu

LGTM. I am not a reviewer. we need other reviewers to approve this.

vnkozlov

Looks good. I verified output.

openjdk · 2022-05-24T14:11:13Z

@aamarsh This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8282024: add EscapeAnalysis statistics under PrintOptoStatistics

Reviewed-by: xliu, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 818 new commits pushed to the master branch:

c674348: 8263075: C2: simplify anti-dependence check in PhaseCFG::implicit_null_check()
0b3d409: 8261768: SelfDestructTimer should accept seconds
bc0379e: 8275303: sun/java2d/pipe/InterpolationQualityTest.java fails with D3D basic render driver
0b8dd4a: 8284966: Update SourceVersion.RELEASE_19 description for language changes
e990fec: 8287089: G1CollectedHeap::is_in_cset() can be const methods
81d7eaf: 8287104: AddressChangeListener thread inherits CCL and can cause memory leak for webapp-servers
796494d: 8284944: assert(cnt++ < 40) failed: infinite cycle in loop optimization
e534c13: 8286398: Address possibly lossy conversions in jdk.internal.le
e21b527: 8228990: JFR: TestNetworkUtilizationEvent.java expects 2+ Network interfaces on Linux but finding 1
e9bddc1: 8262889: Compiler implementation for Record Patterns
... and 808 more: https://git.openjdk.java.net/jdk/compare/f01cce235b62e378e91a3bae32942e2f3dfc5c7e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@navyxliu, @vnkozlov) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

aamarsh · 2022-05-25T12:24:55Z

@vnkozlov It looks like there are two Windows x64 tests failing and after a re-run they have been stuck on the task for 20 hours. The failure happened at:

tools/javac/Paths/MineField.sh
tools/javac/Paths/wcMineField.sh

with the message:

STDERR: Main.java:1: error: cannot find symbol public class Main {public static void main(String[] a) {Lib.f();}}

It does not appear that my code would effect these tests, but I don't want to integrate if the check are are not all passing.

vnkozlov · 2022-05-25T16:53:34Z

@aamarsh Do not worry. I see other latest PRs has the same failure. Your changes should not affect product version of JDK.

aamarsh · 2022-05-25T17:17:14Z

/integrate

openjdk · 2022-05-25T17:31:55Z

@aamarsh
Your change (at version fe3448e) is now ready to be sponsored by a Committer.

JohnTortugo · 2022-06-01T17:51:24Z

Hi, can someone please sponsor this PR if all looks good? TIA!

vnkozlov · 2022-06-01T18:27:50Z

Give me time to quick test to make sure it builds on all our systems. I will sponsor after that.

JohnTortugo · 2022-06-01T18:32:14Z

No worries. Thank you Vladimir!

vnkozlov · 2022-06-01T20:17:43Z

/sponsor

openjdk · 2022-06-01T20:19:53Z

Going to push as commit 2f19144.
Since your change was applied there have been 899 commits pushed to the master branch:

cdb4768: 8287396: LIR_Opr::vreg_number() and data() can return negative number
4caf1ef: 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication
27ad1d5: 8287602: (fs) Avoid redundant HashMap.containsKey call in MimeTypesFileTypeDetector.putIfAbsent
239ac2a: 8286829: Shenandoah: fix Shenandoah Loom support
67ecd30: 8287398: Allow concurrent execution of hotspot docker tests
8071b23: 8287237: (fs) Files.probeContentType returns null if filename contains hash mark on Linux
774928f: 8287625: ProblemList jdk/jshell/HighlightUITest.java on all platforms
e3791ec: 8287491: compiler/jvmci/errors/TestInvalidDebugInfo.java fails new assert: assert((uint)t < T_CONFLICT + 1) failed: invalid type #
f8eb7a8: 8287512: continuationEntry.hpp has incomplete definitions
b2b4ee2: 8287233: Crash in Continuation.enterSpecial: stop: tried to execute native method as non-native
... and 889 more: https://git.openjdk.java.net/jdk/compare/f01cce235b62e378e91a3bae32942e2f3dfc5c7e...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-06-01T20:20:11Z

@vnkozlov @aamarsh Pushed as commit 2f19144.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 29, 2022

aamarsh force-pushed the JDK-8282024 branch from 0c139e9 to e2bcbda Compare March 29, 2022 18:01

JohnTortugo reviewed Mar 29, 2022

View reviewed changes

src/hotspot/share/opto/macro.cpp Outdated Show resolved Hide resolved

JohnTortugo reviewed Mar 29, 2022

View reviewed changes

src/hotspot/share/opto/macro.cpp Outdated Show resolved Hide resolved

JohnTortugo reviewed Mar 29, 2022

View reviewed changes

src/hotspot/share/opto/escape.cpp Outdated Show resolved Hide resolved

JohnTortugo reviewed Mar 29, 2022

View reviewed changes

src/hotspot/share/opto/escape.cpp Outdated Show resolved Hide resolved

aamarsh force-pushed the JDK-8282024 branch from ab603e6 to d98a8df Compare March 31, 2022 18:22

aamarsh marked this pull request as ready for review March 31, 2022 21:12

openjdk bot added the rfr Pull request is ready for review label Mar 31, 2022

aamarsh force-pushed the JDK-8282024 branch 5 times, most recently from be8e235 to 2b7edc4 Compare April 4, 2022 19:03

navyxliu reviewed Apr 10, 2022

View reviewed changes

src/hotspot/share/opto/compile.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/escape.cpp Outdated Show resolved Hide resolved

navyxliu reviewed Apr 10, 2022

View reviewed changes

src/hotspot/share/opto/escape.cpp Outdated Show resolved Hide resolved

aamarsh force-pushed the JDK-8282024 branch from 2b7edc4 to 18328bb Compare May 2, 2022 20:23

navyxliu reviewed May 3, 2022

View reviewed changes

src/hotspot/share/opto/macro.cpp Outdated Show resolved Hide resolved

navyxliu reviewed May 3, 2022

View reviewed changes

src/hotspot/share/opto/compile.cpp Outdated Show resolved Hide resolved

adding escape analysis and scalar replacement statistics

a2811a8

aamarsh force-pushed the JDK-8282024 branch from 18328bb to a2811a8 Compare May 4, 2022 01:32

navyxliu approved these changes May 5, 2022

View reviewed changes

vnkozlov suggested changes May 10, 2022

View reviewed changes

openjdk bot added the rfr Pull request is ready for review label May 20, 2022

vnkozlov reviewed May 20, 2022

View reviewed changes

aamarsh added 2 commits May 20, 2022 12:43

nit picks & removing time

56d6317

make count_MemBar static

8c39455

navyxliu reviewed May 20, 2022

View reviewed changes

aamarsh added 2 commits May 23, 2022 10:39

account for iterative EA

eb84f4d

delete iterative EA comment

fe3448e

navyxliu approved these changes May 24, 2022

View reviewed changes

vnkozlov approved these changes May 24, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 24, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label May 25, 2022

openjdk bot added the integrated Pull request has been integrated label Jun 1, 2022

openjdk bot closed this Jun 1, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jun 1, 2022

8282024: add EscapeAnalysis statistics under PrintOptoStatistics #8019

8282024: add EscapeAnalysis statistics under PrintOptoStatistics #8019

Conversation

aamarsh commented Mar 29, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 29, 2022

openjdk bot commented Mar 29, 2022

mlbridge bot commented Mar 31, 2022 • edited Loading

Webrevs

navyxliu commented Apr 1, 2022 • edited Loading

aamarsh commented Apr 4, 2022

navyxliu commented Apr 5, 2022

navyxliu left a comment

Choose a reason for hiding this comment

navyxliu commented May 4, 2022

navyxliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov commented May 20, 2022

navyxliu May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navyxliu left a comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented May 24, 2022 • edited Loading

aamarsh commented May 25, 2022

vnkozlov commented May 25, 2022

aamarsh commented May 25, 2022

openjdk bot commented May 25, 2022

JohnTortugo commented Jun 1, 2022

vnkozlov commented Jun 1, 2022

JohnTortugo commented Jun 1, 2022

vnkozlov commented Jun 1, 2022

openjdk bot commented Jun 1, 2022

openjdk bot commented Jun 1, 2022

aamarsh commented Mar 29, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 31, 2022 •

edited

Loading

navyxliu commented Apr 1, 2022 •

edited

Loading

navyxliu May 20, 2022 •

edited

Loading

openjdk bot commented May 24, 2022 •

edited

Loading