RFR: 8214455: Relocate CDS archived regions to the top of the G1 heap

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8214455: Relocate CDS archived regions to the top of the G1 heap

Ioi Lam-2
When the runtime heap and dump time heap have different sizes, it's possible that the archived heap regions are mapped to the middle of the runtime heap. Because the archived heap regions are pinned, this could reduce the size of the largest "humongous" array allocation. In the worst case, the maximum allocatable array length may be half of the optimal value.

The fix is to relocate the archived regions to the top of the  G1 heap (currently archived regions are supported only by G1 on 64-bit JVM).

Note that usually the top of the G1 heap is placed just below the 32GB boundary. As a result, the archived heap regions are at the same location between run time and dump time, so no relocation is necessary.

In mach5 testing, we occasionally run into this problem (see https://bugs.openjdk.java.net/browse/JDK-8239089), probably because we fail to reserve the heap below 32GB due to address space layout randomization (ASLR).

-------------

Commit messages:
 - Added -Xlog:cds=debug in case this test fails again
 - 8214455: Relocate CDS archived regions to the top of the G1 heap

Changes: https://git.openjdk.java.net/jdk/pull/3349/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3349&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8214455
  Stats: 143 lines in 8 files changed: 139 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3349.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3349/head:pull/3349

PR: https://git.openjdk.java.net/jdk/pull/3349
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214455: Relocate CDS archived regions to the top of the G1 heap

Thomas Schatzl-4
On Mon, 5 Apr 2021 21:48:43 GMT, Ioi Lam <[hidden email]> wrote:

> When the runtime heap and dump time heap have different sizes, it's possible that the archived heap regions are mapped to the middle of the runtime heap. Because the archived heap regions are pinned, this could reduce the size of the largest "humongous" array allocation. In the worst case, the maximum allocatable array length may be half of the optimal value.
>
> The fix is to relocate the archived regions to the top of the  G1 heap (currently archived regions are supported only by G1 on 64-bit JVM).
>
> Note that usually the top of the G1 heap is placed just below the 32GB boundary. As a result, the archived heap regions are at the same location between run time and dump time, so no relocation is necessary.
>
> In mach5 testing, we occasionally run into this problem (see https://bugs.openjdk.java.net/browse/JDK-8239089), probably because we fail to reserve the heap below 32GB due to address space layout randomization (ASLR).

Lgtm.

src/hotspot/share/memory/filemap.cpp line 1846:

> 1844:       _heap_pointers_need_patching = true;
> 1845:     } else if (header()->heap_end() != CompressedOops::end()) {
> 1846:       log_info(cds)("CDS heap data need to be relocated to the end of the runtime heap to reduce fragmentation");

s/need/needs

test/hotspot/jtreg/runtime/cds/appcds/cacheObject/HeapFragmentationTest.java line 30:

> 28:  * @bug 8214455
> 29:  * @requires vm.cds.archived.java.heap
> 30:  * @requires (sun.arch.data.model == "64"  & os.maxMemory > 4g)

additional space before the &

-------------

Marked as reviewed by tschatzl (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3349
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214455: Relocate CDS archived regions to the top of the G1 heap [v2]

Ioi Lam-2
On Tue, 6 Apr 2021 09:35:01 GMT, Thomas Schatzl <[hidden email]> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   @tschatzl review -- fix typos and whitespace
>
> src/hotspot/share/memory/filemap.cpp line 1846:
>
>> 1844:       _heap_pointers_need_patching = true;
>> 1845:     } else if (header()->heap_end() != CompressedOops::end()) {
>> 1846:       log_info(cds)("CDS heap data need to be relocated to the end of the runtime heap to reduce fragmentation");
>
> s/need/needs

Fixed.

> test/hotspot/jtreg/runtime/cds/appcds/cacheObject/HeapFragmentationTest.java line 30:
>
>> 28:  * @bug 8214455
>> 29:  * @requires vm.cds.archived.java.heap
>> 30:  * @requires (sun.arch.data.model == "64"  & os.maxMemory > 4g)
>
> additional space before the &

Fixed. Thanks for the review.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3349
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214455: Relocate CDS archived regions to the top of the G1 heap [v2]

Calvin Cheung
In reply to this post by Ioi Lam-2
On Tue, 6 Apr 2021 17:04:18 GMT, Ioi Lam <[hidden email]> wrote:

>> When the runtime heap and dump time heap have different sizes, it's possible that the archived heap regions are mapped to the middle of the runtime heap. Because the archived heap regions are pinned, this could reduce the size of the largest "humongous" array allocation. In the worst case, the maximum allocatable array length may be half of the optimal value.
>>
>> The fix is to relocate the archived regions to the top of the  G1 heap (currently archived regions are supported only by G1 on 64-bit JVM).
>>
>> Note that usually the top of the G1 heap is placed just below the 32GB boundary. As a result, the archived heap regions are at the same location between run time and dump time, so no relocation is necessary.
>>
>> In mach5 testing, we occasionally run into this problem (see https://bugs.openjdk.java.net/browse/JDK-8239089), probably because we fail to reserve the heap below 32GB due to address space layout randomization (ASLR).
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>
>   @tschatzl review -- fix typos and whitespace

Looks good. Just one minor nit in the test case.

test/hotspot/jtreg/runtime/cds/appcds/cacheObject/HeapFragmentationTest.java line 84:

> 82:         // Run with CDS. The archived heap regions should be relocated to avoid fragmentation.
> 83:         TestCommon.run(TestCommon.concat(execArgs, runTimeHeapSize, mainClass,  BUFF_SIZE))
> 84:           .assertNormalExit(successOutput);

The assertNormalExit line should be indented four spaces relative to the previous line.

-------------

Marked as reviewed by ccheung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3349
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214455: Relocate CDS archived regions to the top of the G1 heap [v3]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> When the runtime heap and dump time heap have different sizes, it's possible that the archived heap regions are mapped to the middle of the runtime heap. Because the archived heap regions are pinned, this could reduce the size of the largest "humongous" array allocation. In the worst case, the maximum allocatable array length may be half of the optimal value.
>
> The fix is to relocate the archived regions to the top of the  G1 heap (currently archived regions are supported only by G1 on 64-bit JVM).
>
> Note that usually the top of the G1 heap is placed just below the 32GB boundary. As a result, the archived heap regions are at the same location between run time and dump time, so no relocation is necessary.
>
> In mach5 testing, we occasionally run into this problem (see https://bugs.openjdk.java.net/browse/JDK-8239089), probably because we fail to reserve the heap below 32GB due to address space layout randomization (ASLR).

Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:

 - Merge branch 'master' into 8214455-relocated-archived-regions-to-heap-top
 - fixed indentation (review comment by @calvinccheung)
 - @tschatzl review -- fix typos and whitespace
 - Added -Xlog:cds=debug in case this test fails again
 - 8214455: Relocate CDS archived regions to the top of the G1 heap

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3349/files
  - new: https://git.openjdk.java.net/jdk/pull/3349/files/bedd676d..43ee9680

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3349&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3349&range=01-02

  Stats: 4956 lines in 137 files changed: 2977 ins; 1596 del; 383 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3349.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3349/head:pull/3349

PR: https://git.openjdk.java.net/jdk/pull/3349
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214455: Relocate CDS archived regions to the top of the G1 heap [v3]

Ioi Lam-2
In reply to this post by Thomas Schatzl-4
On Tue, 6 Apr 2021 09:41:25 GMT, Thomas Schatzl <[hidden email]> wrote:

>> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
>>
>>  - Merge branch 'master' into 8214455-relocated-archived-regions-to-heap-top
>>  - fixed indentation (review comment by @calvinccheung)
>>  - @tschatzl review -- fix typos and whitespace
>>  - Added -Xlog:cds=debug in case this test fails again
>>  - 8214455: Relocate CDS archived regions to the top of the G1 heap
>
> Lgtm.

Thanks @tschatzl and @calvinccheung for the review.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3349
Reply | Threaded
Open this post in threaded view
|

Integrated: 8214455: Relocate CDS archived regions to the top of the G1 heap

Ioi Lam-2
In reply to this post by Ioi Lam-2
On Mon, 5 Apr 2021 21:48:43 GMT, Ioi Lam <[hidden email]> wrote:

> When the runtime heap and dump time heap have different sizes, it's possible that the archived heap regions are mapped to the middle of the runtime heap. Because the archived heap regions are pinned, this could reduce the size of the largest "humongous" array allocation. In the worst case, the maximum allocatable array length may be half of the optimal value.
>
> The fix is to relocate the archived regions to the top of the  G1 heap (currently archived regions are supported only by G1 on 64-bit JVM).
>
> Note that usually the top of the G1 heap is placed just below the 32GB boundary. As a result, the archived heap regions are at the same location between run time and dump time, so no relocation is necessary.
>
> In mach5 testing, we occasionally run into this problem (see https://bugs.openjdk.java.net/browse/JDK-8239089), probably because we fail to reserve the heap below 32GB due to address space layout randomization (ASLR).

This pull request has now been integrated.

Changeset: 78d1164c
Author:    Ioi Lam <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/78d1164c
Stats:     147 lines in 8 files changed: 139 ins; 0 del; 8 mod

8214455: Relocate CDS archived regions to the top of the G1 heap

Reviewed-by: tschatzl, ccheung

-------------

PR: https://git.openjdk.java.net/jdk/pull/3349