On Thu, 13 Feb 2025 17:45:45 GMT, Ioi Lam <ik...@openjdk.org> wrote: > When running HotSpot jtreg tests in the "AOT mode", for example: > > > make test JTREG_AOT_JDK=true open/test/hotspot/jtreg/runtime/stringtable > > > Before this PR, in the test set up phase, we record several AOT configuration > files by running a few separate Java tools (javac, javap, jlink, and jar), > and then combine them together with sed, grep, sort and uniq: > > https://github.com/openjdk/jdk/blob/adc3f53d2403cd414a91e71c079b4108b2346da0/make/RunTests.gmk#L723-L744 > > After [JDK-8348426](https://bugs.openjdk.org/browse/JDK-8348426), the AOT > configuration file will change to a binary format and can no longer be edited > this way. In preparation for > [JDK-8348426](https://bugs.openjdk.org/browse/JDK-8348426), we should change > the "JTREG_AOT_JDK=true" set up to run a single Java program that > accomplishes the same effect as the current implementation. > > ** Changes in this PR ** > > This PR combines the invocation of these Java tools into a single Java > program, so we just have a single AOT configuration file. It also uses the > `-XX:ExtraSharedClassListFile` option to include the default classlist from > the JDK home directory,
make/RunTests.gmk line 734: > 732: $$(call MakeDir, $$($1_TEST_SUPPORT_DIR)/aot) > 733: > 734: $(info AOT: Create cache configuration) \ Please use logging framework for any messages. To get something to print by default `$(call LogWarn, ...). While here, please also clean up the left over debug $$(info ) left over further down in this file that we weren't able to point out in the original review. make/RunTests.gmk line 737: > 735: $$(call ExecuteWithLog, $$($1_TEST_SUPPORT_DIR)/aot, ( \ > 736: $(CD) $$($1_AOT_JDK_DIR); \ > 737: $(JDK_UNDER_TEST)/bin/javac -d . $(TOPDIR)/make/test/SetupAot.java; > \ This is putting the compilation output in the current directory, which is the make directory in the workspace. That's not acceptable. Since this is a single class java program, would it be possible to just run it without compilation or would that interfere with the intended behavior? Otherwise it needs to be moved to somewhere else, under `$$($1_TEST_SUPPORT_DIR)`. I don't like that the compilation is run in the same recipe as the java command running the tool. At the very least those should be split into separate recipes, with separate ExecuteWithLog calls. A cleaner and preferred solution would be to build this tool as part of the test image so we avoid having to compile code during test setup. make/RunTests.gmk line 741: > 739: -Xlog:cds,cds+class=debug:file=$$($1_AOT_JDK_CONF).log \ > 740: -XX:AOTMode=record > -XX:AOTConfiguration=$$($1_AOT_JDK_CONF) \ > 741: SetupAot \ Please use 4 spaces indent to follow the [Code Conventions for the Build System](https://openjdk.org/groups/build/doc/code-conventions.html). Suggestion: $$(FIXPATH) $(JDK_UNDER_TEST)/bin/java $$($1_VM_OPTIONS) \ -Xlog:cds,cds+class=debug:file=$$($1_AOT_JDK_CONF).log \ -XX:AOTMode=record -XX:AOTConfiguration=$$($1_AOT_JDK_CONF) \ SetupAot \ make/RunTests.gmk line 744: > 742: )) > 743: > 744: $$(info AOT: Generate AOT cache $$($1_AOT_JDK_CACHE) with flags: > $$($1_VM_OPTIONS)) Again, please use the logging framework. In this case I would recommend `$(call LogInfo, ...)` to reduce spam on the default level. I think one line is enough on warn level for indicating that AOT preparation is taking place. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1955089073 PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1955114393 PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1955094235 PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1955090525
