Re: RFR: 8310187: Improve Generational ZGC jtreg testing [v3]
On Mon, 19 Jun 2023 06:55:52 GMT, Axel Boldt-Christmas
wrote:
>> The current implementation for testing generational ZGC with jtreg is
>> implemented with a filter on the mode flag `ZGenerational`. Because of this
>> only environments which set this flag explicitly will run most of the tests.
>> So they get missed in Github Actions and for developers running jtreg
>> locally without supplying the `ZGenerational` flag.
>>
>> The proposed change here is to introduce two new jtreg requirement
>> properties, `vm.gc.ZGenerational` and `vm.gc.ZSingelgen`. These flags will
>> effectively behave the same as the existing `vm.gc.` flags but also take
>> the specific ZGC mode in account.
>>
>> If no gc flags are supplied to jtreg and the `vm.gc.Z` is true (the build
>> includes ZGC) both `vm.gc.ZGenerational` and `vm.gc.ZSinglegen` will be true.
>>
>> If `-XX:+UseZGC` is supplied then both `vm.gc.ZGenerational` and
>> `vm.gc.ZSinglegen` will also be true.
>>
>> If `-XX:{+,-}ZGenerational` is supplied then either `vm.gc.ZGenerational`
>> or `vm.gc.ZSinglegen` be true depending on the flags value.
>>
>> And if `vm.gc.Z` is false both `vm.gc.ZGenerational` and `vm.gc.ZSinglegen`
>> will be false.
>>
>> This change also splits the relevant tests into two distinct runs for the
>> two modes. And the respective test ids are set to `ZGenerational` or
>> `ZSinglegen` to make it easier to distinguish the runs.
>>
>> This also solves the issue that some compiler tests will never run with
>> generational ZGC unless the `TEST_VM_FLAGLESS` is set. This is because the
>> current filter `vm.opt.final.ZGenerational` requires the flag to be
>> explicit, but these compiler tests uses `vm.flagless`.
>>
>> The introduction of `vm.gc.ZGenerational` and `vm.gc.ZSinglegen` harmonizes
>> the way you specify generational / single gen ZGC test with the way it is
>> done for other gcs with `vm.gc.`
>>
>> To support this feature the Whitebox API is extended with `isDefaultVMFlag`
>> to enable checking if `ZGenerational` is default or not.
>>
>> `vm.opt.final.ZGenerational` is still kept because we still have some
>> reasons to filter based on the supplied flags.
>> - `test/hotspot/jtreg/gc/cslocker/TestCSLocker.java` is disabled when
>> running with ZGenerational
>> - `test/jdk/java/lang/ProcessBuilder/CloseRace.java` is ran with a different
>> max heap size for ZGenerational, but it is not the intent to dispatch the
>> test to both G1 and generational ZGC if Generational ZGC is not specified.
>>
>> `test/jdk/java/lang/management/MemoryMXBean/MemoryTest.java` was also
>> changed to not filter but instead dispatch. However u...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add explicit id to all Skynet.java @test
I have no issues with the enhanced Java 2D tests to cover both generational and
original ZGC.
But I'll leave it to hotspot reviews to formally approve the fix since those
tests are such a tiny part of this.
-
PR Comment: https://git.openjdk.org/jdk/pull/14509#issuecomment-1599445397
Re: RFR: 8310187: Improve Generational ZGC jtreg testing [v3]
On Mon, 19 Jun 2023 06:55:52 GMT, Axel Boldt-Christmas
wrote:
>> The current implementation for testing generational ZGC with jtreg is
>> implemented with a filter on the mode flag `ZGenerational`. Because of this
>> only environments which set this flag explicitly will run most of the tests.
>> So they get missed in Github Actions and for developers running jtreg
>> locally without supplying the `ZGenerational` flag.
>>
>> The proposed change here is to introduce two new jtreg requirement
>> properties, `vm.gc.ZGenerational` and `vm.gc.ZSingelgen`. These flags will
>> effectively behave the same as the existing `vm.gc.` flags but also take
>> the specific ZGC mode in account.
>>
>> If no gc flags are supplied to jtreg and the `vm.gc.Z` is true (the build
>> includes ZGC) both `vm.gc.ZGenerational` and `vm.gc.ZSinglegen` will be true.
>>
>> If `-XX:+UseZGC` is supplied then both `vm.gc.ZGenerational` and
>> `vm.gc.ZSinglegen` will also be true.
>>
>> If `-XX:{+,-}ZGenerational` is supplied then either `vm.gc.ZGenerational`
>> or `vm.gc.ZSinglegen` be true depending on the flags value.
>>
>> And if `vm.gc.Z` is false both `vm.gc.ZGenerational` and `vm.gc.ZSinglegen`
>> will be false.
>>
>> This change also splits the relevant tests into two distinct runs for the
>> two modes. And the respective test ids are set to `ZGenerational` or
>> `ZSinglegen` to make it easier to distinguish the runs.
>>
>> This also solves the issue that some compiler tests will never run with
>> generational ZGC unless the `TEST_VM_FLAGLESS` is set. This is because the
>> current filter `vm.opt.final.ZGenerational` requires the flag to be
>> explicit, but these compiler tests uses `vm.flagless`.
>>
>> The introduction of `vm.gc.ZGenerational` and `vm.gc.ZSinglegen` harmonizes
>> the way you specify generational / single gen ZGC test with the way it is
>> done for other gcs with `vm.gc.`
>>
>> To support this feature the Whitebox API is extended with `isDefaultVMFlag`
>> to enable checking if `ZGenerational` is default or not.
>>
>> `vm.opt.final.ZGenerational` is still kept because we still have some
>> reasons to filter based on the supplied flags.
>> - `test/hotspot/jtreg/gc/cslocker/TestCSLocker.java` is disabled when
>> running with ZGenerational
>> - `test/jdk/java/lang/ProcessBuilder/CloseRace.java` is ran with a different
>> max heap size for ZGenerational, but it is not the intent to dispatch the
>> test to both G1 and generational ZGC if Generational ZGC is not specified.
>>
>> `test/jdk/java/lang/management/MemoryMXBean/MemoryTest.java` was also
>> changed to not filter but instead dispatch. However u...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add explicit id to all Skynet.java @test
Thanks for all the reviews.
-
PR Comment: https://git.openjdk.org/jdk/pull/14509#issuecomment-1598489642
Re: RFR: 8310187: Improve Generational ZGC jtreg testing [v3]
On Mon, 19 Jun 2023 09:04:17 GMT, Thomas Stuefe wrote: > How much time do we spend now in GHAs on the additional Zgenerational tests? > In any case, this makes sense. This adds around 40 more tests per platform. The runtime of GHA seems to have a lot of run to run variance. Most new tests are in the `hs/tier1 gc` which has a lower runtime than a few other jobs. So until the heavies jobs gets split into multiple jobs, this should have a negligible affect the overall GHA runtime - PR Comment: https://git.openjdk.org/jdk/pull/14509#issuecomment-1598488556
Re: RFR: 8310187: Improve Generational ZGC jtreg testing [v3]
On Mon, 19 Jun 2023 06:55:52 GMT, Axel Boldt-Christmas
wrote:
>> The current implementation for testing generational ZGC with jtreg is
>> implemented with a filter on the mode flag `ZGenerational`. Because of this
>> only environments which set this flag explicitly will run most of the tests.
>> So they get missed in Github Actions and for developers running jtreg
>> locally without supplying the `ZGenerational` flag.
>>
>> The proposed change here is to introduce two new jtreg requirement
>> properties, `vm.gc.ZGenerational` and `vm.gc.ZSingelgen`. These flags will
>> effectively behave the same as the existing `vm.gc.` flags but also take
>> the specific ZGC mode in account.
>>
>> If no gc flags are supplied to jtreg and the `vm.gc.Z` is true (the build
>> includes ZGC) both `vm.gc.ZGenerational` and `vm.gc.ZSinglegen` will be true.
>>
>> If `-XX:+UseZGC` is supplied then both `vm.gc.ZGenerational` and
>> `vm.gc.ZSinglegen` will also be true.
>>
>> If `-XX:{+,-}ZGenerational` is supplied then either `vm.gc.ZGenerational`
>> or `vm.gc.ZSinglegen` be true depending on the flags value.
>>
>> And if `vm.gc.Z` is false both `vm.gc.ZGenerational` and `vm.gc.ZSinglegen`
>> will be false.
>>
>> This change also splits the relevant tests into two distinct runs for the
>> two modes. And the respective test ids are set to `ZGenerational` or
>> `ZSinglegen` to make it easier to distinguish the runs.
>>
>> This also solves the issue that some compiler tests will never run with
>> generational ZGC unless the `TEST_VM_FLAGLESS` is set. This is because the
>> current filter `vm.opt.final.ZGenerational` requires the flag to be
>> explicit, but these compiler tests uses `vm.flagless`.
>>
>> The introduction of `vm.gc.ZGenerational` and `vm.gc.ZSinglegen` harmonizes
>> the way you specify generational / single gen ZGC test with the way it is
>> done for other gcs with `vm.gc.`
>>
>> To support this feature the Whitebox API is extended with `isDefaultVMFlag`
>> to enable checking if `ZGenerational` is default or not.
>>
>> `vm.opt.final.ZGenerational` is still kept because we still have some
>> reasons to filter based on the supplied flags.
>> - `test/hotspot/jtreg/gc/cslocker/TestCSLocker.java` is disabled when
>> running with ZGenerational
>> - `test/jdk/java/lang/ProcessBuilder/CloseRace.java` is ran with a different
>> max heap size for ZGenerational, but it is not the intent to dispatch the
>> test to both G1 and generational ZGC if Generational ZGC is not specified.
>>
>> `test/jdk/java/lang/management/MemoryMXBean/MemoryTest.java` was also
>> changed to not filter but instead dispatch. However u...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add explicit id to all Skynet.java @test
I'm not the most qualified for ZGC, but I looked this over with a mind on the
planned 21 backport. I wanted to check if any of the existing legacy ZGC tests
had been changed, either accidentally or deliberately. This seems not be the
case.
The remarks are small nits, feel free to ignore them.
How much time do we spend now in GHAs on the additional Zgenerational tests? In
any case, this makes sense.
Cheers, Thomas
test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationTools.java line 298:
> 296: if (selectedGCMode != null) {
> 297: args.add(selectedGCMode);
> 298: }
just to be sure, maybe add a "must not be Z" assert in the else path?
test/hotspot/jtreg/runtime/cds/appcds/TestZGCWithCDS.java line 58:
> 56: public final static String ERR_MSG = "The saved state of
> UseCompressedOops and UseCompressedClassPointers is different from runtime,
> CDS will be disabled.";
> 57: public static void main(String... args) throws Exception {
> 58: String zGenerational = args[0];
assert not null?
-
Marked as reviewed by stuefe (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14509#pullrequestreview-1485730970
PR Review Comment: https://git.openjdk.org/jdk/pull/14509#discussion_r1233738199
PR Review Comment: https://git.openjdk.org/jdk/pull/14509#discussion_r1233740188
Re: RFR: 8310187: Improve Generational ZGC jtreg testing [v3]
> The current implementation for testing generational ZGC with jtreg is
> implemented with a filter on the mode flag `ZGenerational`. Because of this
> only environments which set this flag explicitly will run most of the tests.
> So they get missed in Github Actions and for developers running jtreg locally
> without supplying the `ZGenerational` flag.
>
> The proposed change here is to introduce two new jtreg requirement
> properties, `vm.gc.ZGenerational` and `vm.gc.ZSingelgen`. These flags will
> effectively behave the same as the existing `vm.gc.` flags but also take
> the specific ZGC mode in account.
>
> If no gc flags are supplied to jtreg and the `vm.gc.Z` is true (the build
> includes ZGC) both `vm.gc.ZGenerational` and `vm.gc.ZSingelgen` will be true.
>
> If `-XX:+UseZGC` is supplied then both `vm.gc.ZGenerational` and
> `vm.gc.ZSingelgen` will also be true.
>
> If `-XX:{+,-}ZGenerational` is supplied then either `vm.gc.ZGenerational` or
> `vm.gc.ZSingelgen` be true depending on the flags value.
>
> And if `vm.gc.Z` is false both `vm.gc.ZGenerational` and `vm.gc.ZSingelgen`
> will be false.
>
> This change also splits the relevant tests into two distinct runs for the two
> modes. And the respective test ids are set to `ZGenerational` or `ZSinglegen`
> to make it easier to distinguish the runs.
>
> This also solves the issue that some compiler tests will never run with
> generational ZGC unless the `TEST_VM_FLAGLESS` is set. This is because the
> current filter `vm.opt.final.ZGenerational` requires the flag to be explicit,
> but these compiler tests uses `vm.flagless`.
>
> The introduction of `vm.gc.ZGenerational` and `vm.gc.ZSingelgen` harmonizes
> the way you specify generational / single gen ZGC test with the way it is
> done for other gcs with `vm.gc.`
>
> To support this feature the Whitebox API is extended with `isDefaultVMFlag`
> to enable checking if `ZGenerational` is default or not.
>
> `vm.opt.final.ZGenerational` is still kept because we still have some reasons
> to filter based on the supplied flags.
> - `test/hotspot/jtreg/gc/cslocker/TestCSLocker.java` is disabled when running
> with ZGenerational
> - `test/jdk/java/lang/ProcessBuilder/CloseRace.java` is ran with a different
> max heap size for ZGenerational, but it is not the intent to dispatch the
> test to both G1 and generational ZGC if Generational ZGC is not specified.
>
> `test/jdk/java/lang/management/MemoryMXBean/MemoryTest.java` was also changed
> to not filter but instead dispatch. However unsure if this change should be
> included. The change ...
Axel Boldt-Christmas has updated the pull request incrementally with one
additional commit since the last revision:
Add explicit id to all Skynet.java @test
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14509/files
- new: https://git.openjdk.org/jdk/pull/14509/files/e571b87f..b262d48b
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14509=02
- incr: https://webrevs.openjdk.org/?repo=jdk=14509=01-02
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/14509.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14509/head:pull/14509
PR: https://git.openjdk.org/jdk/pull/14509
