8295867: TestVerifyGraphEdges.java fails with exit code -1073741571 when using AlwaysIncrementalInline #11065

vnkozlov · 2022-11-09T18:14:43Z

When we use -Xcomp we compile java.lang.invoke.LambdaForm$Kind::<clinit> very long linear method for enum class: LambdaForm.java#L250

In addition we inline all class initializers for EA when run with -Xcomp: bytecodeInfo.cpp#L410

Recent CDS change JDK-8293979 allows to inline a bit more deeply too.

Adding -XX:+AlwaysIncrementalInline worsened situation even more.

Running with -XX:+LogCompilation shows that we hit NodeCountInliningCutoff (18000) during java.lang.invoke.LambdaForm$Kind::<clinit> compilation.

In short, we have very long (>40000 live nodes) linear IR graph. Node::verify_edges() method process nodes depth-first starting from first input which is control edge. So it is not surprise that depth of this method recursion reached 6000.
With frame size of 10 words (320 bytes) we easy hit stack overflow (768K in 32-bits debug VM).

I fixed it by using local buffer Node_List instead of recursion in Node::verify_edges().
The algorithm was changed to simplify code. It processes inputs in reverse order - last input processed first. And I noticed that maximum use of buffer is only about 1000 or less elements for this compilation (that is why I use live_nodes/16 as initial size of buffer).

Then I did additional experiment with keeping recursion but processing inputs in reverse order:

   // Recursive walk over all input edges
-  for( i = 0; i < len(); i++ ) {
-    n = in(i);
+  for( i = len(); i > 0; i++ ) {
+    n = in(i - 1);
     if( n != NULL )
       in(i)->verify_edges(visited);
   }

And it shows the same around 1000 stack depth!

I decided to keep my original fix because it should be faster (put only one value on list instead of putting all locals, PC, SP on stack and calls) and much less stack usage.

Testing tier1-3, hs-comp-stress and TestVerifyGraphEdges.java test runs with -XX:+AlwaysIncrementalInline.

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-8295867: TestVerifyGraphEdges.java fails with exit code -1073741571 when using AlwaysIncrementalInline

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer)
Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11065/head:pull/11065
$ git checkout pull/11065

Update a local copy of the PR:
$ git checkout pull/11065
$ git pull https://git.openjdk.org/jdk pull/11065/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11065

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11065.diff

…hen using AlwaysIncrementalInline

bridgekeeper · 2022-11-09T18:15:13Z

👋 Welcome back kvn! 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-11-09T18:19:01Z

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

mlbridge · 2022-11-09T18:33:02Z

Webrevs

01: Full - Incremental (b74d1215)
00: Full (2978787e)

chhagedorn

I agree that a non-recursive solution is preferable in this case. I only have some minor code style comments - otherwise, the fix looks good!

chhagedorn · 2022-11-10T06:55:07Z

src/hotspot/share/opto/compile.cpp

+    uint stack_size = live_nodes() >> 4;
+    Node_List nstack(MAX2(stack_size, (uint)OptoNodeListSize));


As you only need the stack in verify_edges(), I suggest to move these lines directly into the method verify_edges().

I need live_nodes() value or stack_size or C to pass for creating list inside method.
I decided to move renamed verify_bidirectional_edges() method to Compile class to get these values inside the method.
It does not need to be in Node class after I removed recursion.

Right, I've missed that before. Moving it to Compile is a good idea to apply that!

src/hotspot/share/opto/node.cpp

chhagedorn · 2022-11-10T07:09:56Z

src/hotspot/share/opto/node.cpp

+    // Walk over all input edges, checking for correspondence
+    uint length = next->len();
+    for (uint i = 0; i < length; i++) {
+      Node* n = next->in(i);


I suggest to rename n to input to make it easier to see if it is the current node or an input to it.

Renamed to in

src/hotspot/share/opto/node.hpp

openjdk · 2022-11-10T07:21:21Z

@vnkozlov 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:

8295867: TestVerifyGraphEdges.java fails with exit code -1073741571 when using AlwaysIncrementalInline

Reviewed-by: chagedorn, shade

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 12 new commits pushed to the master branch:

12e76cb: 8296349: [aarch64] Avoid slicing Address::extend
7244eac: 8296771: RISC-V: C2: assert(false) failed: bad AD file
956d75b: 8295099: vmTestbase/nsk/stress/strace/strace013.java failed with "TestFailure: wrong lengths of stack traces: strace013Thread0: NNN strace013Thread83: MMM"
2f9a94f: 8296824: ProblemList compiler/jvmci/jdk.vm.ci.code.test/src/jdk/vm/ci/code/test/NativeCallTest.java
43ebd96: 8296822: ProblemList jdk/jfr/api/consumer/TestRecordingFileWrite.java
84e1224: 8296496: Overzealous check in sizecalc.h prevents large memory allocation
27527b4: 8296612: CertAttrSet is useless
6b456f7: 8262901: [macos_aarch64] NativeCallTest expected:<-3.8194101E18> but was:<3.02668882E10>
e1badb7: 8295871: G1: Use different explicit claim marks for CLDs
9ef7852: 8290714: Make com.sun.jndi.dns.DnsClient virtual threads friendly
... and 2 more: https://git.openjdk.org/jdk/compare/4a68210d9f6c59ec4289b2e2412a1ae0df17fd81...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

shipilev

x86_32 seems to be happy with this change. I ran both TestVerifyGraphEdges and tier1 tier2 with -XX:+VerifyGraphEdges without problems with Linux x86_32 fastdebug. The code looks reasonable too.

vnkozlov · 2022-11-10T16:30:25Z

Thank you, Aleksey and Christian.
I am testing changes requested by Christian.

chhagedorn

Thanks for doing the updates, looks good!

vnkozlov · 2022-11-11T16:07:48Z

/integrate

openjdk · 2022-11-11T16:09:51Z

Going to push as commit 819c691.
Since your change was applied there have been 19 commits pushed to the master branch:

ced88a2: 8296733: JFR: File Read event for RandomAccessFile::write(byte[]) is incorrect
87b809a: 8296229: JFR: jfr tool should print unsigned values correctly
e7c2a8e: 8295214: Generational ZGC: Guard nmethods from cross modifying code
d4d183e: 8296301: Interpreter(RISC-V): Implement -XX:+PrintBytecodeHistogram and -XX:+PrintBytecodePairHistogram options
f754840: 8296773: G1: Factor out hash function for G1CardSet
fdabd37: 8293696: java/nio/channels/DatagramChannel/SelectWhenRefused.java fails with "Unexpected wakeup"
4a30081: 8296747: com/sun/net/httpserver/simpleserver/StressDirListings.java timed out
12e76cb: 8296349: [aarch64] Avoid slicing Address::extend
7244eac: 8296771: RISC-V: C2: assert(false) failed: bad AD file
956d75b: 8295099: vmTestbase/nsk/stress/strace/strace013.java failed with "TestFailure: wrong lengths of stack traces: strace013Thread0: NNN strace013Thread83: MMM"
... and 9 more: https://git.openjdk.org/jdk/compare/4a68210d9f6c59ec4289b2e2412a1ae0df17fd81...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-11T16:10:16Z

@vnkozlov Pushed as commit 819c691.

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

8295867: TestVerifyGraphEdges.java fails with exit code -1073741571 w…

96031bf

…hen using AlwaysIncrementalInline

Merge branch 'master' into JDK-8295867

6ea8de6

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 9, 2022

remove white space

2978787

vnkozlov marked this pull request as ready for review November 9, 2022 18:26

openjdk bot added the rfr Pull request is ready for review label Nov 9, 2022

chhagedorn approved these changes Nov 10, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 10, 2022

shipilev approved these changes Nov 10, 2022

View reviewed changes

Merge branch 'master' into JDK-8295867

d760070

Rename verify_edges() method and move it to Compile class

b74d121

shipilev approved these changes Nov 10, 2022

View reviewed changes

vnkozlov requested a review from chhagedorn November 11, 2022 00:01

chhagedorn approved these changes Nov 11, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 11, 2022

openjdk bot closed this Nov 11, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 11, 2022

vnkozlov deleted the JDK-8295867 branch November 11, 2022 16:18

vpaprotsk mentioned this pull request Nov 14, 2022

8288047: Accelerate Poly1305 on x86_64 using AVX512 instructions #10582

Closed

3 tasks

This was referenced Nov 16, 2022

8296170: Refactor stack-locking path in C2_MacroAssembler::fast_unlock() #10936

Closed

JDK-8296784: Provide clean mallinfo/mallinfo2 wrapper for Linux glibc platforms #11088

Closed

8295867: TestVerifyGraphEdges.java fails with exit code -1073741571 when using AlwaysIncrementalInline #11065

8295867: TestVerifyGraphEdges.java fails with exit code -1073741571 when using AlwaysIncrementalInline #11065

vnkozlov commented Nov 9, 2022 •

edited by openjdk bot

bridgekeeper bot commented Nov 9, 2022

openjdk bot commented Nov 9, 2022

mlbridge bot commented Nov 9, 2022 •

edited

chhagedorn left a comment

chhagedorn Nov 10, 2022

vnkozlov Nov 10, 2022 •

edited

chhagedorn Nov 11, 2022

chhagedorn Nov 10, 2022

vnkozlov Nov 10, 2022

openjdk bot commented Nov 10, 2022 •

edited

shipilev left a comment

vnkozlov commented Nov 10, 2022

chhagedorn left a comment

vnkozlov commented Nov 11, 2022

openjdk bot commented Nov 11, 2022

openjdk bot commented Nov 11, 2022

		uint stack_size = live_nodes() >> 4;
		Node_List nstack(MAX2(stack_size, (uint)OptoNodeListSize));

8295867: TestVerifyGraphEdges.java fails with exit code -1073741571 when using AlwaysIncrementalInline #11065

8295867: TestVerifyGraphEdges.java fails with exit code -1073741571 when using AlwaysIncrementalInline #11065

Conversation

vnkozlov commented Nov 9, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 9, 2022

openjdk bot commented Nov 9, 2022

mlbridge bot commented Nov 9, 2022 • edited

Webrevs

chhagedorn left a comment

Choose a reason for hiding this comment

chhagedorn Nov 10, 2022

Choose a reason for hiding this comment

vnkozlov Nov 10, 2022 • edited

Choose a reason for hiding this comment

chhagedorn Nov 11, 2022

Choose a reason for hiding this comment

chhagedorn Nov 10, 2022

Choose a reason for hiding this comment

vnkozlov Nov 10, 2022

Choose a reason for hiding this comment

openjdk bot commented Nov 10, 2022 • edited

shipilev left a comment

Choose a reason for hiding this comment

vnkozlov commented Nov 10, 2022

chhagedorn left a comment

Choose a reason for hiding this comment

vnkozlov commented Nov 11, 2022

openjdk bot commented Nov 11, 2022

openjdk bot commented Nov 11, 2022

vnkozlov commented Nov 9, 2022 •

edited by openjdk bot

mlbridge bot commented Nov 9, 2022 •

edited

vnkozlov Nov 10, 2022 •

edited

openjdk bot commented Nov 10, 2022 •

edited