-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8373023: [REDO] Remove the default value of InitialRAMPercentage #28641
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
Conversation
|
👋 Welcome back jsikstro! A progress list of the required criteria for merging this PR into |
|
@jsikstro 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 1 new commit pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
| # jdk_jdi | ||
|
|
||
| com/sun/jdi/InvokeHangTest.java 8218463 linux-all | ||
| com/sun/jdi/MethodInvokeWithTraceOnTest.java 8373022 generic-all |
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.
What about java/lang/management/MemoryPoolMXBean/ThresholdTest.java, that was the test that started failing for me yesterday.
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.
The test failure in java/lang/management/MemoryPoolMXBean/ThresholdTest.java is fixed in #28630
This was a pre-existing issue that is not directly tied to this change.
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, I didn't see that change go by.
03bdb9e to
6a3510b
Compare
kstefanj
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.
Looks good
|
@jsikstro Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
Thank you for the reviews everyone! |
|
Going to push as commit 8d80778.
Your commit was automatically rebased without conflicts. |
Hello,
We backed out this change in #28617 after issues in found in testing. Moving forward, we've decided to problem list a few tests that assumes that a GC does not run during their execution. When the initial heap size becomes very low with this change, it may trigger additional GCs, which may interfere with a few tests.
We are planning to follow up on the problem lists with test fixes in:
#28638, #28637
From the original RFE:
This RFE changes the default of
InitialRAMPercentageto 0, effectively removing its default value. Please see the CSR for specific details on this change.Changing the default value to 0 results in the behavior that the initial heap size (InitialHeapSize) is set to the minimum heap size (MinHeapSize). This is because of the following lines in
Arguments::set_heap_size(), which takes the MAX value of MinHeapSize as well:This change improves startup performance for all GCs, but affects the time-to-peak performance in some out-of-the-box configurations for some GCs. This is mainly visible in ParallelGC.
Testing:
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28641/head:pull/28641$ git checkout pull/28641Update a local copy of the PR:
$ git checkout pull/28641$ git pull https://git.openjdk.org/jdk.git pull/28641/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28641View PR using the GUI difftool:
$ git pr show -t 28641Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28641.diff
Using Webrev
Link to Webrev Comment