-
Notifications
You must be signed in to change notification settings - Fork 147
8267271: Fix gc/arguments/TestNewRatioFlag.java expectedNewSize calculation #28
Conversation
👋 Welcome back sjohanss! A progress list of the required criteria for merging this PR into |
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. Took a while to find the corresponding code in the VM :)
@kstefanj 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 75 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 |
Yes, that's why I opted to add the comment about it. =) Thanks for reviewing. |
Yes, I could have read the entire fix before diving into the code after seeing the deletions... ;) |
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.
Thanks @tschatzl and @kimbarrett for the reviews. /integrate |
Going to push as commit 59de99d.
Your commit was automatically rebased without conflicts. |
Please review this test-fix to avoid failures on certain platforms.
Summary
Fixes an incorrect calculation in the test when trying to predict the new size given the
NewRatio
. The fix is not strictly bound to different platforms but to the resulting heap-size the test is run with. Both with Serial and Parallel the test fails if run on a platform using a heap alignment that forces the heap to be 104 MB instead of the specified 100 MB. It is a bit unclear why the test even passes with 100MB, since the calculation done in the test doesn't match the calculation done in the JVM.Testing
Manual testing to verify the fix actually fixes the issue and works with multiple page sizes. Mach5 testing on affected platforms to verify the fix is good there as well.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/28/head:pull/28
$ git checkout pull/28
Update a local copy of the PR:
$ git checkout pull/28
$ git pull https://git.openjdk.java.net/jdk17 pull/28/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28
View PR using the GUI difftool:
$ git pr show -t 28
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/28.diff