-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8370519: C2: Hit MemLimit when running with +VerifyLoopOptimizations #28581
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
base: master
Are you sure you want to change the base?
Conversation
|
👋 Welcome back roland! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@rwestrel |
Webrevs
|
mhaessig
left a comment
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.
Thank you for fixing this, @rwestrel. Your fix looks good to me. I merely have two nitpicky suggestions.
I will kick off a run of testing and report back with the results.
| * @test | ||
| * @bug 8370519 | ||
| * @summary C2: Hit MemLimit when running with +VerifyLoopOptimizations |
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.
Unsure, but would this test qualify for @key stress?
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.
I'm not sure either what does.
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.
It is a marker to filter resource intensive tests.
jdk/test/hotspot/jtreg/TEST.ROOT
Lines 29 to 30 in 7278d2e
| # The list of keywords supported in this test suite | |
| # stress: stress/slow test |
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.
I added it in the new commit.
src/hotspot/share/opto/compile.hpp
Outdated
|
|
||
| // Compilation environment. | ||
| Arena* comp_arena() { return &_comp_arena; } | ||
| ResourceArea* idealloop_arena() { return &_idealloop_arena; } |
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.
Should we make it more idiomatic C++ by having the ResourceArea allocated and deallocated together with the PhaseIdealLoop instead of attaching it to the Compile object?
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.
Right, that makes sense. Done in new commits.
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 for fixing this @rwestrel, I agree with the solution. I noticed that this could be a problem while working on JDK-8366990, but there was no reproducer at the time.
src/hotspot/share/opto/compile.cpp
Outdated
| _clinit_barrier_on_entry(false), | ||
| _stress_seed(0), | ||
| _comp_arena(mtCompiler, Arena::Tag::tag_comp), | ||
| _idealloop_arena(mtCompiler, Arena::Tag::tag_idealloop), |
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.
To keep the naming consistent with other mentions of IdealLoop in variable/field names (such as _phase_verify_ideal_loop), I would name this _ideal_loop_arena. This will make it easier to find in a code editor. Feel free to ignore if you disagree
| * @test | ||
| * @bug 8370519 | ||
| * @summary C2: Hit MemLimit when running with +VerifyLoopOptimizations | ||
| * @run main/othervm -XX:CompileCommand=compileonly,*TestVerifyLoopOptimizationsHighMemUsage*::* -XX:-TieredCompilation -Xbatch |
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.
Out of curiosity, have you try reducing the test with creduce? I fixed a similar issue in JDK-8366990, and initially reviewers were concerned about the long compilation time. I was able to get decent results with creduce by using -XX:CompileCommand=memlimit. Not sure if it's worth doing here though.
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.
I don't have creduce set up. I tried minimizing the test case by hand but it was fairly time consuming. It currently runs in 30s on a fairly fast machine.
|
Fwiw, testing passed up to tier3 on linux-x64, linux-aarch64, macosx-aarch64, mac-x64, windows-x64. |
Co-authored-by: Manuel Hässig <[email protected]>
For this failure memory stats are:
The noticeable line is:
A lot of memory (almost 1 GB) gets allocated in the
comparenaduring
idealLoop. So even though the compilation goes over the limitin
Compile::Code_Gen(), the root cause is what happens earlier,during
idealLoop._loop_or_ctrland_bodyare both allocated in thecomparena. Accumulated over several loop opts pass, they should not use
that much memory but the test is run with
+VerifyLoopOptimizations:calls to
PhaseIdealLoop::verify()cause newPhaseIdealLoopobjectsto be allocated and more memory to be used in the
comparena. Thefix I propose is to allocate
_loop_or_ctrland_bodyin adedicated
ResourceAreaso memory can be reclaimed when a pass ofloop opts is over.
With that change:
that is ~50MB total for
idealLoopinstead of almost 1GB. Total usagepeaks around 200MB.
/cc hotspot-compiler
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28581/head:pull/28581$ git checkout pull/28581Update a local copy of the PR:
$ git checkout pull/28581$ git pull https://git.openjdk.org/jdk.git pull/28581/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28581View PR using the GUI difftool:
$ git pr show -t 28581Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28581.diff
Using Webrev
Link to Webrev Comment