-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8326615: C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash) #19280
base: master
Are you sure you want to change the base?
Conversation
…nal Error (codeBlob.cpp:429) Initial size of CodeCache is too small
👋 Welcome back dfenacci! A progress list of the required criteria for merging this PR into |
@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:
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 298 new commits pushed to the
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 |
@dafedafe this pull request can not be integrated into git checkout JDK-8326615
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 |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a regression in JDK 23, right? Could you please adjust the affects versions in JIRA accordingly?
Looks good to me otherwise.
Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>
Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>
It is not. Fixing the version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
src/hotspot/share/c1/c1_Runtime1.cpp
Outdated
@@ -280,6 +285,7 @@ void Runtime1::initialize(BufferBlob* blob) { | |||
#endif | |||
BarrierSetC1* bs = BarrierSet::barrier_set()->barrier_set_c1(); | |||
bs->generate_c1_runtime_stubs(blob); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to handle failures in generate_c1_runtime_stubs? With the assert removed, I think we'll get a nullptr crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you're right. Actually that call could potentially fail too.
I've added code to handle that case as well.
Thanks @dean-long!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me otherwise.
Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>
Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>
Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>
Thanks for the review @TobiHartmann! |
This looks OK, but isn't it a lot of changes just to get this test to pass? Aren't all of these allocation failures ultimately fatal? Is there a simpler way to handle this problem? |
It seems a bit much indeed but I think there is potentially always the possibility of not failing but only disabling the compiler. @dean-long do you think the VM would anyway fail later on? |
It may not fail, but if it can't create a C1 or C2 compiler, then that's bad, and we might argue that this kind of failure should be similar to a failure during JVM startup. In fact, I have been thinking that there are reasons why we might want these compiler stubs to be created earlier in startup, when we are still single-threaded. That would get rid of any races, and make a failure fatal. It would also allow us to allow us to put these stubs, if they are effectively execute-only because they don't need to be patched, into a special JIT region, avoiding the MAP_JIT overhead on macos-aarch64 (see JDK-8331978). |
Issue
The test
compiler/startup/StartupOutput.java
fails intermittently due to a crash after correctly printing the errorInitial size of CodeCache is too small
(the test limits the code cache using k-XX:InitialCodeCacheSize=1024K -XX:ReservedCodeCacheSize=1200k`).The appearance of the issue is very dependent on thread scheduling. The original report happens during C1 initialization but C2 initialization is affected as well.
Causes
There is one occurrence during C1 initialization and one during C2 initialization where a call to
RuntimeStub::new_runtime_stub
can fail fatally if there is not enough space left.For C1:
Compiler::init_c1_runtime
->Runtime1::initialize
->Runtime1::generate_blob_for
->Runtime1::generate_blob
->RuntimeStub::new_runtime_stub
.For C2:
C2Compiler::initialize
->OptoRuntime::generate
->OptoRuntime::generate_stub
->Compile::Compile
->Compile::Code_Gen
->PhaseOutput::install
->PhaseOutput::install_stub
->RuntimeStub::new_runtime_stub
.Solution
#15970 introduced an optional argument to
RuntimeStub::new_runtime_stub
to determine if it fails fatally or not. We can take advantage of it to avoid crashing and instead pass the information about the success or failure of the allocation up the (C1 and C2 initialization) call stack up to where we can set the compilations as failed.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19280/head:pull/19280
$ git checkout pull/19280
Update a local copy of the PR:
$ git checkout pull/19280
$ git pull https://git.openjdk.org/jdk.git pull/19280/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19280
View PR using the GUI difftool:
$ git pr show -t 19280
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19280.diff
Webrev
Link to Webrev Comment