RFR: 8274166: Some CDS tests ignore -Dtest.cds.runtime.options

[David Holmes]

On Fri, 31 Mar 2023 18:20:55 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

> The test.cds.runtime.options property is used to execute the CDS tests in a special mode. E.g., create the archived with G1 but load the archive with EpsilonGC.
> 
> Currently tests that are launched with TestCommon.runWithArchive() and CDSTestUtils.runWithArchive() can handle -Dtest.cds.runtime.options. However, some test cases, such as dynamicArchive/HelloDynamic.java, do not call the above two methods, so they ignore -Dtest.cds.runtime.options.
> 
> The fix is to move the handling of -Dtest.cds.runtime.options to TestCommon.executeAndLog, because most CDS tests use this function to launch a child JVM. Other considerations are necessary now that the option is exposed to other tests, so checks for GC and CDS are necessary. Verified with tier1-4 tests.

I'm not clear how the GC options are actually combined. Are you saying there are some tests that explicitly use a given GC, and if these also use test.cds.runtime.options, which contains a GC option, then the original value is kept and the latter is ignored? Won't that lead to redundant test execution? i.e. if we set test.cds.runtime.options to have UseZGC and then run all the CDS tests, those that explicitly use G1 will continue to use G1, but that configuration would already have been tested  by a test run that didn't set test.cds.runtime.options.

A few suggested code code simplifications.

Thanks.

test/lib/jdk/test/lib/cds/CDSTestUtils.java line 609:

> 607:     // Check commandline for the last instance of Xshare to see if the process can load
> 608:     // a CDS archive
> 609:     public static boolean maybeRunningWithArchive(List<String> cmd) {

`isRunningWithArchive` seems more appropriate

test/lib/jdk/test/lib/cds/CDSTestUtils.java line 615:

> 613:       }
> 614: 
> 615:       Pattern share = Pattern.compile("-Xshare:.*");

This can be a static so it only needs to be compiled once.

test/lib/jdk/test/lib/cds/CDSTestUtils.java line 624:

> 622:       }
> 623: 
> 624:       if (lastXShare.equals("-Xshare:dump") || lastXShare.equals("-Xshare:off")) {

Seems slightly clearer to just do:

return lastXShare.equals("-Xshare:auto") || lastXShare.equals("-Xshare:on");

test/lib/jdk/test/lib/cds/CDSTestUtils.java line 631:

> 629: 
> 630:     static boolean maybeAddOption(String cmd, boolean hasGCOption, ArrayList<String> cmdline) {
> 631:       Pattern gc = Pattern.compile("^-XX:\\+.*GC");

This could be a static variable so it can be used by different methods and only compiled once.

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13273#pullrequestreview-1368150086
PR Review Comment: https://git.openjdk.org/jdk/pull/13273#discussion_r1155385133
PR Review Comment: https://git.openjdk.org/jdk/pull/13273#discussion_r1155384791
PR Review Comment: https://git.openjdk.org/jdk/pull/13273#discussion_r1155384722
PR Review Comment: https://git.openjdk.org/jdk/pull/13273#discussion_r1155384580