8347406: [REDO] C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash) #23630

dafedafe · 2025-02-14T11:04:20Z

Issue

The test src/hotspot/share/opto/c2compiler.cpp fails intermittently due to a crash that happens when trying to allocate code cache space for C1 and C2 in RuntimeStub::new_runtime_stub and SingletonBlob::operator new.

Causes

There are a few call paths during the initialization of C1 and C2 that can lead to the code cache allocations in RuntimeStub::new_runtime_stub (through RuntimeStub::operator new) and SingletonBlob::operator new triggering a fatal error if there is no more space. The paths in question are:

Compiler::init_c1_runtime -> Runtime1::initialize -> Runtime1::generate_blob_for -> Runtime1::generate_blob -> RuntimeStub::new_runtime_stub
C2Compiler::initialize -> C2Compiler::init_c2_runtime -> OptoRuntime::generate -> OptoRuntime::generate_stub -> Compile::Compile -> Compile::Code_Gen -> PhaseOutput::install -> PhaseOutput::install_stub -> RuntimeStub::new_runtime_stub
C2Compiler::initialize -> C2Compiler::init_c2_runtime -> OptoRuntime::generate -> OptoRuntime::generate_uncommon_trap_blob -> UncommonTrapBlob::create -> new UncommonTrapBlob
C2Compiler::initialize -> C2Compiler::init_c2_runtime -> OptoRuntime::generate -> OptoRuntime::generate_exception_blob -> ExceptionBlob::create -> new ExceptionBlob

Solution

Instead of fatally crashing the we can use the alloc_fail_is_fatal flag of RuntimeStub::new_runtime_stub to avoid crashing in cases 1 and 2 and add a similar flag to SingletonBlob::operator new for cases 3 and 4. In the latter case we need to adjust all calls accordingly.

Note: In JDK-8326615 it was argued that increasing the minimum code cache size would solve the issue but that wasn't entirely accurate: doing so possibly decreases the chances of a failed allocation in these 4 places but doesn't totally avoid it.

Testing

The original failing regression test in test/hotspot/jtreg/compiler/startup/StartupOutput.java has been modified to run multiple times with randomized values (within the original failing range) to increase the chances of hitting the fatal assertion.

Tests: Tier 1-4 (windows-x64, linux-x64/aarch64, and macosx-x64/aarch64; release and debug mode)

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-8347406: [REDO] C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash) (Bug - P4)

Reviewers

Dean Long (@dean-long - Reviewer)
Andrew Dinn (@adinn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23630

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…nal Error (codeBlob.cpp:429) Initial size of CodeCache is too small

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

… more cases)

…ime::generate

bridgekeeper · 2025-02-14T11:08:52Z

👋 Welcome back dfenacci! 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 · 2025-02-14T11:09:18Z

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

8347406: [REDO] C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash)

Reviewed-by: dlong, adinn

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

5709f79: 8352648: JFR: 'jfr query' should not be available in product builds
02a4ce2: 8352147: G1: TestEagerReclaimHumongousRegionsClearMarkBits test takes very long
de58009: 8351468: C2: array fill optimization assigns wrong type to intrinsic call
a875733: 8352486: [ubsan] compilationMemoryStatistic.cpp:659:21: runtime error: index 64 out of bounds for type const struct unnamed struct
5591f8a: 8351515: C2 incorrectly removes double negation for double and float
56a4ffa: 8352597: [IR Framework] test bug: TestNotCompilable.java fails on product build
e23e0f8: 8352591: Missing UnlockDiagnosticVMOptions in VerifyGraphEdgesWithDeadCodeCheckFromSafepoints test
adfb120: 8351748: Add class init barrier to AOT-cached Method/Var Handles
ee1577b: 8352652: [BACKOUT] nsk/jvmti/ tests should fail when nsk_jvmti_setFailStatus() is called
df9210e: 8347706: jvmciEnv.cpp has jvmci includes out of order
... and 378 more: https://git.openjdk.org/jdk/compare/efbad00c4d7931177ccc5e9bce3b30dfbac94010...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.

openjdk · 2025-02-14T11:09:59Z

@dafedafe this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8347406
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2025-02-14T11:10:12Z

@dafedafe The following labels will be automatically applied to this pull request:

hotspot
shenandoah

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

mlbridge · 2025-02-20T16:32:18Z

Webrevs

dafedafe · 2025-02-20T16:43:48Z

@adinn I noticed I touched quite a few runtime files that you recently refactored. I guess it might make sense if you had a look at them. Thanks!

dean-long · 2025-02-20T20:53:30Z

I don't understand why JDK-8326615 didn't work. If the minimum codecache size was too small, can't we just increase it?

src/hotspot/share/c1/c1_Runtime1.cpp

dean-long · 2025-02-20T21:34:16Z

src/hotspot/share/gc/shenandoah/c1/shenandoahBarrierSetC1.cpp

  }
+  return _pre_barrier_c1_runtime_code_blob != nullptr && reference_barrier_success;


Wouldn't it be better to return false immediately after each failure, rather than continuing?

Yes, totally. Fixed.

dean-long · 2025-02-20T21:35:20Z

src/hotspot/share/gc/z/c1/zBarrierSetC1.cpp

@@ -540,4 +540,8 @@ void ZBarrierSetC1::generate_c1_runtime_stubs(BufferBlob* blob) {
    generate_c1_store_runtime_stub(blob, true /* self_healing */, "store_barrier_on_oop_field_with_healing");
  _store_barrier_on_oop_field_without_healing =
    generate_c1_store_runtime_stub(blob, false /* self_healing */, "store_barrier_on_oop_field_without_healing");
+  return _load_barrier_on_oop_field_preloaded_runtime_stub != nullptr &&


Again, why not return false immediately on first failure?

dean-long · 2025-02-20T21:36:44Z

src/hotspot/share/opto/output.cpp

+      if (rs == nullptr) {
+        C->record_failure("CodeCache is full");
+      } else {
+        C->set_stub_entry_point(rs->entry_point());


Is the deleted rs->is_runtime_stub() assert still useful here?

A slightly modified one surely is. Inserted it again.

I was thinking it could be moved into the else clause and simplified further.

Oh I see 👍. Moved. Thanks!

dafedafe · 2025-02-27T08:29:35Z

I don't understand why JDK-8326615 didn't work. If the minimum codecache size was too small, can't we just increase it?

It seems that the small minimum codecache size wasn’t the core of the problem. In JDK-8326615 I’ve actually tried increasing the minimum code cache size (even calculating the minimum C1 and C2 sizes needed) but I kept hitting the same problem seemingly because what triggers the crash is the code cache exceeding its limits exactly during one of the allocations in the 4 call paths listed in the description (any allocation at another point simply turns off C1 or C2 (or both)). Increasing the minimum was possibly making it a bit less probable (i.e. when running something small that would use very little code cache, e.g. java -version) but as soon as running something a bit bigger (requiring more code cache), we could still end up calling the problematic allocations right when the code cache was full.
BTW even if the crash was very dependent on C1/C2 thread scheduling etc., I was able to reproduce it with different minimum code cache sizes relatively frequently.

Even so, it might be a good idea to additionally increase the minimum code cache anyway. @dean-long do you think it would make sense to file an RFE for that?

Co-authored-by: Dean Long <17332032+dean-long@users.noreply.github.com>

dean-long · 2025-02-28T20:35:58Z

Refreshing my memory, isn't the real problem with trying to fix this with a minimum codecache size is that some of these stubs are not allocated during initial single-threaded JVM startup, but later when the first compiler threads start, and that allows other code blobs to fill up the codecache?

dean-long · 2025-02-28T20:36:23Z

Even so, it might be a good idea to additionally increase the minimum code cache anyway. @dean-long do you think it would make sense to file an RFE for that?

Sure, if it's still an issue.

dafedafe · 2025-03-03T12:40:55Z

Refreshing my memory, isn't the real problem with trying to fix this with a minimum codecache size is that some of these stubs are not allocated during initial single-threaded JVM startup, but later when the first compiler threads start, and that allows other code blobs to fill up the codecache?

Yes, exactly. This seems to be even more of an issue with 2 compiler threads (i.e. C1/C2) since the first can fill up the code cache first at the expense of the other.
The result is that if one compiler thread tries to allocate more space in a full code cache during initialization with one of the 4 call paths above, the VM crashes (but could actually just turn off the compiler thread instead).

adinn · 2025-03-24T15:24:32Z

@dean-long You are right that the root of the problem is delayed stub init when running in tiered mode. It's actually a race between the C1 and C2 threads to consume code cache space.

Neither C1 nor C2 can generate compiled method code before they have initialized their respective stubs. However, initialization of C2 stubs may potentially occur after a C1 compiler thread has initialized its own stubs and generated some number of C1-compiled methods.

Clearly that can't happen the other way around because in tiered mode a C2 thread will only end up compiling a method after 1) a prior C1-compiled method exists in the runtime and 2) its own stubs have been generated. An implication of 1) is that the C1 stubs have already been generated.

dean-long

Looks good!

adinn

Changes look good to me too.

dafedafe · 2025-03-25T07:09:47Z

Thank you very much for your reviews @dean-long and @adinn!

dafedafe · 2025-03-25T07:10:07Z

/integrate

openjdk · 2025-03-25T07:10:50Z

Going to push as commit 48fac66.
Since your change was applied there have been 400 commits pushed to the master branch:

99c8a6e: 8350463: AArch64: Add vector rearrange support for small lane count vectors
b2da0d3: 8352289: [macos] Review skipped tests in tools/jpackage/macosx/SigningPackage*
ba658a7: 8349522: AArch64: Add backend implementation for new unsigned and saturating vector operations
5625b43: 8350429: runtime/NMT/CheckForProperDetailStackTrace.java should only run for debug JVM
2c60fc5: 8352176: Automate setting up environment for mac signing tests
6e6a39d: 8347321: [ubsan] CGGlyphImages.m:553:30: runtime error: nan is outside the range of representable values of type 'unsigned long'
b84b292: 8352615: [Test] RISC-V: TestVectorizationMultiInvar.java fails on riscv64 without rvv support
a54445f: 8350609: Cleanup unknown unwind opcode (0xB) for windows
c87e1be: 8349582: APX NDD code generation for OpenJDK
7d1fe0e: 8339543: [vectorapi] laneHelper and withLaneHelper should be ForceInline
... and 390 more: https://git.openjdk.org/jdk/compare/efbad00c4d7931177ccc5e9bce3b30dfbac94010...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-25T07:11:05Z

@dafedafe Pushed as commit 48fac66.

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

dafedafe and others added 13 commits February 10, 2025 15:30

Update src/hotspot/share/c1/c1_Compiler.cpp

50c4929

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

Update src/hotspot/share/c1/c1_Runtime1.cpp

f38f7f4

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

JDK-8326615: handle allocation failures in barrier set

163f7ab

Update src/hotspot/share/gc/z/c1/zBarrierSetC1.cpp

e56c702

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

Update src/hotspot/share/gc/z/c1/zBarrierSetC1.cpp

e17d0a3

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

JDK-8347406: avoid crashing if code cache allocation comes from C2 (2…

bcc5db2

… more cases)

JDK-8347406: remove merge leftover method

5e5c949

JDK-8347406: reduce number of startup test attempts

c4be79c

JDK-8347406: make startup processes run in parallel

Loading
Loading status checks…

a26a461

JDK-8347406: reduce number of processes in test

f002464

JDK-8347406: fix c2 runtime init return condition

7a691a5

JDK-8347406: set the C2 uncommon and exception trap blobs in OptoRunt…

Loading
Loading status checks…

cd4aa4d

…ime::generate

openjdk bot added the merge-conflict label Feb 14, 2025

openjdk bot added hotspot shenandoah labels Feb 14, 2025

dafedafe added 4 commits February 17, 2025 13:55

JDK-8347406: reduce number of test processes

Loading
Loading status checks…

81b5a15

Merge branch 'master' into JDK-8347406

Loading
Loading status checks…

47ed354

Merge branch 'master' into JDK-8347406

b38560a

JDK-8347406: update copyright year

Loading
Loading status checks…

3a492cf

openjdk bot removed the merge-conflict label Feb 19, 2025

JDK-8347406: reduce number of tests again

Loading
Loading status checks…

e930df4

dafedafe marked this pull request as ready for review February 20, 2025 16:27

openjdk bot added the rfr label Feb 20, 2025

dean-long reviewed Feb 20, 2025

View reviewed changes

src/hotspot/share/c1/c1_Runtime1.cpp Outdated Show resolved Hide resolved

dean-long reviewed Feb 20, 2025

View reviewed changes

dafedafe and others added 3 commits February 27, 2025 09:31

Update src/hotspot/share/c1/c1_Runtime1.cpp

Loading
Loading status checks…

85eb102

Co-authored-by: Dean Long <17332032+dean-long@users.noreply.github.com>

JDK-8347406: return immediately if stub generation fails

Loading
Loading status checks…

26a6747

JDK-8347406: re-add modified assert

Loading
Loading status checks…

906cd75

JDK-8347406: move assert into else clause

Loading
Loading status checks…

722ca50

dean-long approved these changes Mar 24, 2025

View reviewed changes

openjdk bot added the ready label Mar 24, 2025

adinn approved these changes Mar 24, 2025

View reviewed changes

openjdk bot added the integrated label Mar 25, 2025

openjdk bot closed this Mar 25, 2025

openjdk bot removed ready rfr labels Mar 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8347406: [REDO] C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash) #23630

8347406: [REDO] C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash) #23630

dafedafe commented Feb 14, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 14, 2025

openjdk bot commented Feb 14, 2025 •

edited

Loading

openjdk bot commented Feb 14, 2025

openjdk bot commented Feb 14, 2025

mlbridge bot commented Feb 20, 2025 •

edited

Loading

dafedafe commented Feb 20, 2025

dean-long commented Feb 20, 2025

dean-long Feb 20, 2025

dafedafe Feb 27, 2025

dean-long Feb 20, 2025

dafedafe Feb 27, 2025

dean-long Feb 20, 2025

dafedafe Feb 27, 2025

dean-long Feb 28, 2025

dafedafe Mar 3, 2025

dafedafe commented Feb 27, 2025

dean-long commented Feb 28, 2025

dean-long commented Feb 28, 2025

dafedafe commented Mar 3, 2025

adinn commented Mar 24, 2025

dean-long left a comment

adinn left a comment

dafedafe commented Mar 25, 2025

dafedafe commented Mar 25, 2025

openjdk bot commented Mar 25, 2025

openjdk bot commented Mar 25, 2025

		}
		return _pre_barrier_c1_runtime_code_blob != nullptr && reference_barrier_success;

8347406: [REDO] C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash) #23630

8347406: [REDO] C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash) #23630

Conversation

dafedafe commented Feb 14, 2025 • edited by openjdk bot Loading

Issue

Causes

Solution

Testing

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 14, 2025

openjdk bot commented Feb 14, 2025 • edited Loading

openjdk bot commented Feb 14, 2025

openjdk bot commented Feb 14, 2025

mlbridge bot commented Feb 20, 2025 • edited Loading

Webrevs

dafedafe commented Feb 20, 2025

dean-long commented Feb 20, 2025

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

Choose a reason for hiding this comment

dafedafe commented Feb 27, 2025

dean-long commented Feb 28, 2025

dean-long commented Feb 28, 2025

dafedafe commented Mar 3, 2025

adinn commented Mar 24, 2025

dean-long left a comment

Choose a reason for hiding this comment

adinn left a comment

Choose a reason for hiding this comment

dafedafe commented Mar 25, 2025

dafedafe commented Mar 25, 2025

openjdk bot commented Mar 25, 2025

openjdk bot commented Mar 25, 2025

dafedafe commented Feb 14, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 14, 2025 •

edited

Loading

mlbridge bot commented Feb 20, 2025 •

edited

Loading