8268127: Shenandoah: Heap size may be too small for region to align to large page size #4320

zhengyu123 · 2021-06-02T20:47:32Z

TestLargePages.java failed on Linux AArch64, where large page size is 500M, and the test is configured to run with much smaller heap, e.g. 131M.

This is not AArch64 specific, just none of our regular test machines has large page setup.

I purpose two invariants:

Heap size always aligns up to page size that is requested.
Region size always aligns up to page size that is requested if heap is large enough to accommodate them. Otherwise, it aligns up to regular page size.

This partially reverts JDK-8266802.

Obviously, this is a corner case and is unlikely seen in real word. The alternative is to disable UseLargePages if heap size is too small, e.g. align_up(heap_size, page_size) < MIN_NUM_REGIONS.

Test:
hotspot_gc_shenandoah

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8268127: Shenandoah: Heap size may be too small for region to align to large page size

Reviewers

Roman Kennke (@rkennke - Reviewer) ⚠️ Review applies to e2c6a99
Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to e2c6a99

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4320/head:pull/4320
$ git checkout pull/4320

Update a local copy of the PR:
$ git checkout pull/4320
$ git pull https://git.openjdk.java.net/jdk pull/4320/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4320

View PR using the GUI difftool:
$ git pr show -t 4320

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4320.diff

bridgekeeper · 2021-06-02T20:48:27Z

👋 Welcome back zgu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-06-02T20:51:51Z

@zhengyu123 The following labels will be automatically applied to this pull request:

hotspot-gc
shenandoah

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-06-02T20:55:51Z

Webrevs

rkennke

Please check what is up with pre-submit tests.

rkennke · 2021-06-07T12:23:20Z

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.cpp

@@ -467,7 +467,7 @@ size_t ShenandoahHeapRegion::block_size(const HeapWord* p) const {
  }
 }

-void ShenandoahHeapRegion::setup_sizes(size_t max_heap_size) {
+void ShenandoahHeapRegion::setup_sizes(size_t& max_heap_size) {


I wonder if it would be clearer to return the adjusted max_heap_size instead?

shipilev

This looks fine to me, modulo nits below. Consider adding new test cases in gc/shenandoah/TestLargePages.java. This fails for me on Linux x86_64 (2M large pages by default): build/linux-x86_64-server-fastdebug/images/jdk/bin/java -XX:+UseShenandoahGC -Xmx16m -XX:+UseLargePages Test

shipilev · 2021-06-08T13:52:34Z

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

+  log_info(gc)("Setup heap, size: " SIZE_FORMAT "K, region size: " SIZE_FORMAT "K and region count: " SIZE_FORMAT,
+              max_byte_size / K, reg_size_bytes / K, _num_regions);


This seems redundant. I think gc+init already prints this?

shipilev · 2021-06-08T13:59:14Z

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.cpp

+      page_size = (int)large_page_size;
+    } else {
+      // Should have been checked during argument initialization
+      assert(!ShenandoahUncommit, "Uncommit requires region size aligns to underline page size");


Assert message: "Uncommit requires region size aligned to large page size"?

openjdk · 2021-06-08T14:07:47Z

@zhengyu123 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:

8268127: Shenandoah: Heap size may be too small for region to align to large page size

Reviewed-by: rkennke, shade

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 8 new commits pushed to the master branch:

51e8201: 8267764: jpackage cannot handle window screensaver files when EXE renamed as SCR
f9b593d: 8266748: Move modifiers code to Signatures.java
4dd0e7e: 8259806: Clean up terminology on the "All Classes" page
dc6c96b: 8263468: New page for "recent" new API
fafc4d9: 8268352: Rename javadoc Messager class to JavadocLog
b568e87: 8237388: serviceability/dcmd/framework/VMVersionTest.java fails with connection refused error.
c21cc93: 8248877: Document API contract for MetaspaceObj subtypes
159cb6f: 8268083: JDK-8267706 breaks bin/idea.sh on a Mac

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 master branch, type /integrate in a new comment.

zhengyu123 · 2021-06-08T20:29:54Z

/integrate

openjdk · 2021-06-08T20:31:41Z

@zhengyu123 Since your change was applied there have been 9 commits pushed to the master branch:

7a37816: 8264866: Remove unneeded WorkArounds.isAutomaticModule
51e8201: 8267764: jpackage cannot handle window screensaver files when EXE renamed as SCR
f9b593d: 8266748: Move modifiers code to Signatures.java
4dd0e7e: 8259806: Clean up terminology on the "All Classes" page
dc6c96b: 8263468: New page for "recent" new API
fafc4d9: 8268352: Rename javadoc Messager class to JavadocLog
b568e87: 8237388: serviceability/dcmd/framework/VMVersionTest.java fails with connection refused error.
c21cc93: 8248877: Document API contract for MetaspaceObj subtypes
159cb6f: 8268083: JDK-8267706 breaks bin/idea.sh on a Mac

Your commit was automatically rebased without conflicts.

Pushed as commit 5ad4a91.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Zhengyu Gu added 2 commits June 2, 2021 12:31

v1

ed6dd78

v2

892d07b

openjdk bot added the rfr Pull request is ready for review label Jun 2, 2021

openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Jun 2, 2021

Zhengyu Gu added 2 commits June 2, 2021 17:54

Fix compilation error

1e0e832

Window build

b5e0137

rkennke reviewed Jun 7, 2021

View reviewed changes

Zhengyu Gu added 2 commits June 7, 2021 08:42

Merge branch 'master' into JDK-8268127-large-page-small-heap

7f47674

rkennke's comment

e2c6a99

shipilev approved these changes Jun 8, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 8, 2021

Zhengyu Gu added 3 commits June 8, 2021 10:44

Aleksey's comment

f5e4faf

Merge branch 'master' into JDK-8268127-large-page-small-heap

bc5a129

Added extra line

d490e06

rkennke approved these changes Jun 8, 2021

View reviewed changes

openjdk bot closed this Jun 8, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8268127: Shenandoah: Heap size may be too small for region to align to large page size #4320

8268127: Shenandoah: Heap size may be too small for region to align to large page size #4320

zhengyu123 commented Jun 2, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 2, 2021

openjdk bot commented Jun 2, 2021

mlbridge bot commented Jun 2, 2021 •

edited

Loading

rkennke left a comment

rkennke Jun 7, 2021

shipilev left a comment

shipilev Jun 8, 2021

shipilev Jun 8, 2021

openjdk bot commented Jun 8, 2021 •

edited

Loading

zhengyu123 commented Jun 8, 2021

openjdk bot commented Jun 8, 2021

		log_info(gc)("Setup heap, size: " SIZE_FORMAT "K, region size: " SIZE_FORMAT "K and region count: " SIZE_FORMAT,
		max_byte_size / K, reg_size_bytes / K, _num_regions);

8268127: Shenandoah: Heap size may be too small for region to align to large page size #4320

8268127: Shenandoah: Heap size may be too small for region to align to large page size #4320

Conversation

zhengyu123 commented Jun 2, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 2, 2021

openjdk bot commented Jun 2, 2021

mlbridge bot commented Jun 2, 2021 • edited Loading

Webrevs

rkennke left a comment

Choose a reason for hiding this comment

rkennke Jun 7, 2021

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

shipilev Jun 8, 2021

Choose a reason for hiding this comment

shipilev Jun 8, 2021

Choose a reason for hiding this comment

openjdk bot commented Jun 8, 2021 • edited Loading

zhengyu123 commented Jun 8, 2021

openjdk bot commented Jun 8, 2021

zhengyu123 commented Jun 2, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 2, 2021 •

edited

Loading

openjdk bot commented Jun 8, 2021 •

edited

Loading