On Mon, 19 Jun 2023 06:55:52 GMT, Axel Boldt-Christmas <abold...@openjdk.org>
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.<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.<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
