RFR: 8264540: WhiteBox.metaspaceReserveAlignment should return shared region alignment

[Yumin Qi-3]

Hi, Please review
  After JDK-8236847, the shared region alignment (new as MetaspaceShared::core_region_alignment) is no longer default to os pagesize, it is a configurable value at build time instead. The WhiteBox api metaspaceReserveAlignment() should reflect the change.

Tests:tier1,tier2,tier3,tier4

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

Commit messages:
 - 8264540: WhiteBox.metaspaceReserveAlignment should return shared region alignment

Changes: https://git.openjdk.java.net/jdk/pull/3309/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3309&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264540
  Stats: 23 lines in 5 files changed: 8 ins; 3 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3309.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3309/head:pull/3309

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

[Yumin Qi-3]

> Hi, Please review
>   After JDK-8236847, the shared region alignment (new as MetaspaceShared::core_region_alignment) is no longer default to os pagesize, it is a configurable value at build time instead. The WhiteBox api metaspaceReserveAlignment() should reflect the change.
>
> Tests:tier1,tier2,tier3,tier4

Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:

  Fix minimal build failure

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3309/files
  - new: https://git.openjdk.java.net/jdk/pull/3309/files/c8d80a05..f99b1c90

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

  Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3309.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3309/head:pull/3309

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

[Calvin Cheung]

On Thu, 1 Apr 2021 22:15:51 GMT, Yumin Qi <[hidden email]> wrote:

>> Hi, Please review
>>   After JDK-8236847, the shared region alignment (new as MetaspaceShared::core_region_alignment) is no longer default to os pagesize, it is a configurable value at build time instead. The WhiteBox api metaspaceReserveAlignment() should reflect the change.
>>
>> Tests:tier1,tier2,tier3,tier4
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix minimal build failure

Looks good. Just one comment in one of the test changes.

test/hotspot/jtreg/runtime/cds/appcds/SharedRegionAlignmentTest.java line 38:

> 36:  * @run driver jdk.test.lib.helpers.ClassFileInstaller sun.hotspot.WhiteBox
> 37:  * @run driver jdk.test.lib.helpers.ClassFileInstaller -jar hello.jar Hello
> 38:  * @run main/othervm/timeout=240 -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -Xbootclasspath/a:. SharedRegionAlignmentTest

Is the increase in timeout necessary?

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

Marked as reviewed by ccheung (Reviewer).

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

[Ioi Lam-2]

On Thu, 1 Apr 2021 22:15:51 GMT, Yumin Qi <[hidden email]> wrote:

>> Hi, Please review
>>   After JDK-8236847, the shared region alignment (new as MetaspaceShared::core_region_alignment) is no longer default to os pagesize, it is a configurable value at build time instead. The WhiteBox api metaspaceReserveAlignment() should reflect the change.
>>
>> Tests:tier1,tier2,tier3,tier4
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix minimal build failure

Changes requested by iklam (Reviewer).

test/hotspot/jtreg/runtime/cds/appcds/SharedRegionAlignmentTest.java line 75:

> 73:                                                  TestCommon.concat(dumpLP, logArg));
> 74:             out.shouldContain("Dumping shared data to file");
> 75:             boolean is_alignment_logged = out.getStdout().contains(regionAlignmentString + regionAlignment);

Is `is_alignment_logged` still needed? Now both dump time and run time should contain the `regionAlignmentString + regionAlignment` string. Also, I think it's better to use a local variable like:

String expectedAlignment = regionAlignmentString + regionAlignment;

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

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

[Yumin Qi-3]

On Fri, 2 Apr 2021 16:37:51 GMT, Calvin Cheung <[hidden email]> wrote:

>> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix minimal build failure
>
> test/hotspot/jtreg/runtime/cds/appcds/SharedRegionAlignmentTest.java line 38:
>
>> 36:  * @run driver jdk.test.lib.helpers.ClassFileInstaller sun.hotspot.WhiteBox
>> 37:  * @run driver jdk.test.lib.helpers.ClassFileInstaller -jar hello.jar Hello
>> 38:  * @run main/othervm/timeout=240 -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -Xbootclasspath/a:. SharedRegionAlignmentTest
>
> Is the increase in timeout necessary?

I will do a test in debug to check if it should be removed. Thanks.

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

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

[Yumin Qi-3]

On Fri, 2 Apr 2021 16:45:39 GMT, Ioi Lam <[hidden email]> wrote:

>> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix minimal build failure
>
> test/hotspot/jtreg/runtime/cds/appcds/SharedRegionAlignmentTest.java line 75:
>
>> 73:                                                  TestCommon.concat(dumpLP, logArg));
>> 74:             out.shouldContain("Dumping shared data to file");
>> 75:             boolean is_alignment_logged = out.getStdout().contains(regionAlignmentString + regionAlignment);
>
> Is `is_alignment_logged` still needed? Now both dump time and run time should contain the `regionAlignmentString + regionAlignment` string. Also, I think it's better to use a local variable like:
>
> String expectedAlignment = regionAlignmentString + regionAlignment;

Yes, it should be contained in both dump/runtime time, will remove it. Thanks.

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

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

[Yumin Qi-3]

> Hi, Please review
>   After JDK-8236847, the shared region alignment (new as MetaspaceShared::core_region_alignment) is no longer default to os pagesize, it is a configurable value at build time instead. The WhiteBox api metaspaceReserveAlignment() should reflect the change.
>
> Tests:tier1,tier2,tier3,tier4

Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:

  SharedRegionAlignmentTest: removed timeout on test, removed is_alignment_logged

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3309/files
  - new: https://git.openjdk.java.net/jdk/pull/3309/files/f99b1c90..155b3b7d

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

  Stats: 10 lines in 1 file changed: 1 ins; 3 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3309.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3309/head:pull/3309

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

[Calvin Cheung]

On Fri, 2 Apr 2021 17:31:39 GMT, Yumin Qi <[hidden email]> wrote:

>> Hi, Please review
>>   After JDK-8236847, the shared region alignment (new as MetaspaceShared::core_region_alignment) is no longer default to os pagesize, it is a configurable value at build time instead. The WhiteBox api metaspaceReserveAlignment() should reflect the change.
>>
>> Tests:tier1,tier2,tier3,tier4
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>
>   SharedRegionAlignmentTest: removed timeout on test, removed is_alignment_logged

Marked as reviewed by ccheung (Reviewer).

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

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

[Ioi Lam-2]

On Fri, 2 Apr 2021 17:31:39 GMT, Yumin Qi <[hidden email]> wrote:

>> Hi, Please review
>>   After JDK-8236847, the shared region alignment (new as MetaspaceShared::core_region_alignment) is no longer default to os pagesize, it is a configurable value at build time instead. The WhiteBox api metaspaceReserveAlignment() should reflect the change.
>>
>> Tests:tier1,tier2,tier3,tier4
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>
>   SharedRegionAlignmentTest: removed timeout on test, removed is_alignment_logged

Marked as reviewed by iklam (Reviewer).

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

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

[Yumin Qi-3]

On Thu, 1 Apr 2021 16:41:15 GMT, Yumin Qi <[hidden email]> wrote:

> Hi, Please review
>   After JDK-8236847, the shared region alignment (new as MetaspaceShared::core_region_alignment) is no longer default to os pagesize, it is a configurable value at build time instead. The WhiteBox api metaspaceReserveAlignment() should reflect the change.
>
> Tests:tier1,tier2,tier3,tier4

This pull request has now been integrated.

Changeset: d920f858
Author:    Yumin Qi <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/d920f858
Stats:     32 lines in 5 files changed: 14 ins; 5 del; 13 mod

8264540: WhiteBox.metaspaceReserveAlignment should return shared region alignment

Reviewed-by: ccheung, iklam

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

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

[Yumin Qi-3]

On Fri, 2 Apr 2021 17:47:16 GMT, Calvin Cheung <[hidden email]> wrote:

>> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   SharedRegionAlignmentTest: removed timeout on test, removed is_alignment_logged
>
> Marked as reviewed by ccheung (Reviewer).

@calvinccheung @iklam Thanks for review!

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

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