-
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
8330647: Two CDS tests fail with -UseCompressedOops and UseSerialGC/UseParallelGC #19274
8330647: Two CDS tests fail with -UseCompressedOops and UseSerialGC/UseParallelGC #19274
Conversation
👋 Welcome back ccheung! A progress list of the required criteria for merging this PR into |
@calvinccheung 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 160 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 |
@calvinccheung The following label will be automatically applied to this pull request:
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. |
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.
Can't the code that implements the @requires
check also check for that property?
Yes, I've reverted the changes in the tests and added the checking of the |
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.
Update looks good. One query below. Thanks
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.
LGTM.
test/jtreg-ext/requires/VMProps.java
Outdated
for (String opt : jtropts.split(",")) { | ||
if (opt.equals(CCP_DISABLED)) | ||
return false; | ||
if (opt.startsWith(GC_PREFIX) && opt.endsWith(GC_SUFFIX) && | ||
!opt.equals(G1GC_ENABLED)) | ||
return false; |
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.
Based on the bug report both of these conditions are needed to be enabled for there to be a problem, but you exclude based on either -XX:-UseCompressedClassPointers"
or any non-G1 GC. ??
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've checked again, without the fix, the test fails if running with either -Dtest.cds.runtime.options=-XX:-UseCompressedClassPointers
or any non-G1GC such as -Dtest.cds.runtime.options=-XX:+UseZGC
.
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 review of the functional changes of this fix, but since this patch does change the shared VMProps.java file I have some style requests.
* @return true if the VM options specified via the "test.cds.runtime.options" | ||
* property is compatible with writing Java heap objects into the CDS archive | ||
*/ | ||
protected boolean isCDSRuntimeOptionsCompatible() { |
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.
Please add braces for all the added if
statements.
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.
Fixed.
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.
You missed this one:
if (jtropts == null)
return true;
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.
Fixed.
* @return true if the VM options specified via the "test.cds.runtime.options" | ||
* property is compatible with writing Java heap objects into the CDS archive | ||
*/ | ||
protected boolean isCDSRuntimeOptionsCompatible() { |
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.
You missed this one:
if (jtropts == null)
return true;
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.
Okay that seems fine if either condition disables writing into the CDS archive.
Thanks
Thanks @iklam @dholmes-ora @stefank @turbanoff for the review. /integrate |
Going to push as commit c2cca2a.
Your commit was automatically rebased without conflicts. |
@calvinccheung Pushed as commit c2cca2a. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The following two tests contain
@requires vm.cds.write.archived.java.heap
which impliesUseG1GC && UseCompressedClassPointers
The tests would fail if options conflicting with
vm.cds.write.archived.java.heap
are specified via the-Dtest.cds.runtime.options
property because the options would be added after the@require
check.A fix is to check if the
-Dtest.cds.runtime.options
property contains non-null value and throws aSkippedException
.Update:
The checking of the
-Dtest.cds.runtime.options
property is now performed in VMProps.java.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19274/head:pull/19274
$ git checkout pull/19274
Update a local copy of the PR:
$ git checkout pull/19274
$ git pull https://git.openjdk.org/jdk.git pull/19274/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19274
View PR using the GUI difftool:
$ git pr show -t 19274
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19274.diff
Webrev
Link to Webrev Comment