8354145: G1: UseCompressedOops boundary is calculated on maximum heap region size instead of maxiumum ergonomic heap region size

[tbzhang]

After JDK-8275056, The max heap region size became 512M, and the calculation of CompressedOops based on the max_heap_size - max_heap_region_size.
So before this patch, the CompressedOops will turn on below 32G - 32m, After this patch is 32G -512m.

When our Apps migrating from JDK11 to JDK21, the heap size parameters(Xmx32736m) will turn off the CompressedOops.

Since the current max ergonomics size is still 32m, We hoped that the original behavior will not be changed if HeapRegionSize is not explicitly set.

before this patch:

./build/linux-x86_64-server-release/images/jdk/bin/java -Xmx32736m -XX:+PrintFlagsFinal -version | grep CompressedOops
     bool UseCompressedOops                        = false                          {product lp64_product} {default}
openjdk version "25-internal" 2025-09-16
OpenJDK Runtime Environment (build 25-internal-adhoc.root.jdk)
OpenJDK 64-Bit Server VM (build 25-internal-adhoc.root.jdk, mixed mode, sharing)

after this patch:

./build/linux-x86_64-server-release/images/jdk/bin/java -Xmx32736m -XX:+PrintFlagsFinal -version | grep CompressedOops
     bool UseCompressedOops                        = true                           {product lp64_product} {ergonomic}
openjdk version "25-internal" 2025-09-16
OpenJDK Runtime Environment (build 25-internal-adhoc.root.jdk)
OpenJDK 64-Bit Server VM (build 25-internal-adhoc.root.jdk, mixed mode, sharing)

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8354145: G1: UseCompressedOops boundary is calculated on maximum heap region size instead of maxiumum ergonomic heap region size (Bug - P4)

Reviewers

• Thomas Schatzl (@tschatzl - Reviewer)
• Albert Mingkun Yang (@albertnetymk - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24541/head:pull/24541
$ git checkout pull/24541

Update a local copy of the PR:
$ git checkout pull/24541
$ git pull https://git.openjdk.org/jdk.git pull/24541/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24541

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24541.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back tbzhang! 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]

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

8354145: G1: UseCompressedOops boundary is calculated on maximum heap region size instead of maxiumum ergonomic heap region size

Reviewed-by: tschatzl, ayang

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

• 375ac6d: 8355796: RISC-V: compiler/vectorapi/AllBitsSetVectorMatchRuleTest.java fails after JDK-8355657
• 1a9a928: 8355651: Issues with post-image hook
• dcffd9d: 8347471: Provide valid flags and mask in AccessFlag.Location
• ... and 363 more: https://git.openjdk.org/jdk/compare/cd9fa3f7aa0324c575943deebb41f4f7ff4f73d3...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@tschatzl, @lmesnik, @albertnetymk) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@tbzhang The following label will be automatically applied to this pull request:

• hotspot-gc

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.

[mlbridge]

Webrevs

• 03: Full - Incremental (17c0a8a0)
• 02: Full - Incremental (f08e4177)
• 01: Full - Incremental (c31c7340)
• 00: Full (6b139085)

[tschatzl]

Would it be possible to add a regression test that checks the value of the UseCompressedOops flag after running a VM with these settings?

[tbzhang]

Would it be possible to add a regression test that checks the value of the UseCompressedOops flag after running a VM with these settings?

Thanks for your suggestion, I will add a test.

[openjdk]

@tbzhang 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.

[lmesnik]

Why this line is needed? I don't see any dependencies on "/"
If you use some test code outside directory, better to build them.

[tbzhang]

Why this line is needed? I don't see any dependencies on "/" If you use some test code outside directory, better to build them.

Yes, the GCArguments depends on the @library / , many tests in test/hotspot/jtreg/gc/arguments use this

[tschatzl]

Afaict GCArguments only depends on test.lib.* too.
Other than that, the use of the @librarydirectives is often just copy&paste without particular meaning.

[lmesnik]

Is any of those 2 modules is used by tests? I don't see it in the test.

[tbzhang]

removed these two modules

[lmesnik]

The test ignores external VM flags, so vm.opt.G1HeapRegionSize is not needed.
But it is needed to add
* @requires vm.flagless

[tbzhang]

done

[lmesnik]

Sorry, I wanted to ask you to change test, not approve it yet.

[tbzhang]

Sorry, I wanted to ask you to change test, not approve it yet.

Got it, thanks for the review.

[tschatzl]

Afaict GCArguments only depends on test.lib.* too.
Other than that, the use of the @librarydirectives is often just copy&paste without particular meaning.

[tstuefe]

This may be a stupid question, but why does the heap region size factor into this decision at all? I assume that both heap base and heap max size are aligned to heap region size?

[albertnetymk]

why does the heap region size factor into this decision at all?

I wonder if it's due to Arguments::max_heap_for_compressed_oops:

  // We need to fit both the null page and the heap into the memory budget, while
  // keeping alignment constraints of the heap. To guarantee the latter, as the
  // null page is located before the heap, we pad the null page to the conservative
  // maximum alignment that the GC may ever impose upon the heap.
  size_t displacement_due_to_null_page = align_up(os::vm_page_size(),
                                                  _conservative_max_heap_alignment);

  LP64_ONLY(return OopEncodingHeapMax - displacement_due_to_null_page);

The actual heap starts after the null-page, and the null-page takes the heap-region size in order for heap-start to be aligned.

[tschatzl]

This may be a stupid question, but why does the heap region size factor into this decision at all? I assume that both heap base and heap max size are aligned to heap region size?

From the calculation for max heap for compressed oops:

size_t Arguments::max_heap_for_compressed_oops() {
  // Avoid sign flip.
  assert(OopEncodingHeapMax > (uint64_t)os::vm_page_size(), "Unusual page size");
  // We need to fit both the null page and the heap into the memory budget, while
  // keeping alignment constraints of the heap. To guarantee the latter, as the
  // null page is located before the heap, we pad the null page to the conservative
  // maximum alignment that the GC may ever impose upon the heap.
  size_t displacement_due_to_null_page = align_up(os::vm_page_size(),
                                                  _conservative_max_heap_alignment);

  LP64_ONLY(return OopEncodingHeapMax - displacement_due_to_null_page);
  NOT_LP64(ShouldNotReachHere(); return 0);
}

This conservative max heap alignment is what we change from absolute maximum (512M) to what the ergonomics would at most use (32M). At that point we can only use these conservative values, because ergonomics only later decides heap region size based on max heap size which the code may not have at this point.
(Afair)

[tstuefe]

why does the heap region size factor into this decision at all?

I wonder if it's due to Arguments::max_heap_for_compressed_oops:

  // We need to fit both the null page and the heap into the memory budget, while
  // keeping alignment constraints of the heap. To guarantee the latter, as the
  // null page is located before the heap, we pad the null page to the conservative
  // maximum alignment that the GC may ever impose upon the heap.
  size_t displacement_due_to_null_page = align_up(os::vm_page_size(),
                                                  _conservative_max_heap_alignment);

  LP64_ONLY(return OopEncodingHeapMax - displacement_due_to_null_page);

The actual heap starts after the null-page, and the null-page takes the heap-region size in order for heap-start to be aligned.

Thanks @albertnetymk @tbzhang, that makes sense:

• Null area must be located at encoding base (heap base)
• Heap is split into regions, regions must be pow2-sized and (I assume) start at region-size-aligned addresses.
• Null area must directly precede first region. First region follows Null area. So Null area must be sized to be region size.

In Metaspace, I do this differently:

jdk/src/hotspot/share/memory/metaspace.cpp

Line 816 in 6a0c24f

if (CompressedKlassPointers::base() == (address)rs.base()) {

Here, the Null area is part of the first Root chunk segment. I set up class space starting at encoding base, then allocate a smaller area from that chunk, protect it, and never free it again.

I very briefly wondered why this was not possible in the java heap (e.g. protect the first page of the first region) but that would be a worse solution for many reasons. It would mean region 0 requires special attention, would prevent it from being collected normally, would mean we had to find a separate solution for every collector etc. Rather just live with wasting a bit of address space at the front of the heap.

[albertnetymk]

This PR is good by itself.

Maybe in a followup, one can explore the possibility of adjusting (e.g. align-down instead) max-heap-size somewhere in order to avoid the current circular dependency.

[tbzhang]

Thanks for the review!

[tbzhang]

/integrate

[openjdk]

@tbzhang
Your change (at version 17c0a8a) is now ready to be sponsored by a Committer.

[albertnetymk]

/sponsor

[openjdk]

Going to push as commit 526951d.
Since your change was applied there have been 369 commits pushed to the master branch:

• 765cef4: 8355878: RISC-V: jdk/incubator/vector/DoubleMaxVectorTests.java fails when using RVV
• d802fd0: 8352422: [ubsan] Out-of-range reported in ciMethod.cpp:917:20: runtime error: 2.68435e+09 is outside the range of representable values of type 'int'
• 7d9a438: 8355370: Include server name in HTTP test server thread names to improve diagnostics
• ... and 366 more: https://git.openjdk.org/jdk/compare/cd9fa3f7aa0324c575943deebb41f4f7ff4f73d3...master

Your commit was automatically rebased without conflicts.

[openjdk]

@albertnetymk @tbzhang Pushed as commit 526951d.

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