RFR(M) 8152988: [AOT] Update test batch definitions to include aot-ed java.base module mode into hs-comp testing

[Ekaterina Pavlova]

Magnus,
thanks a lot for your review.

Regarding "-J-Xmx4g --info".
This is done intentionally. AOT requires more heap usage and could fail with OOM otherwise.
I specify "mem.total" in AOT jobs definitions so AOT testing is done only on machines which has at least 4gb.

-katya

On 10/19/18 12:21 AM, Magnus Ihse Bursie wrote:
> On 2018-10-18 08:29, Ekaterina Pavlova wrote:
> 
>> Hi All,
>>
>> Please review the changes which add support to build AOTed jdk modules and then use them during the testing.
>> Note, building of AOTed libraries needs to be done at the same machine the testing is going to be started.
>> So, this could not be done at jdk build time.
>>
>> Updated files:
>> - make/RunTests.gmk, make/RunTestsPrebuilt.gmk, make/RunTestsPrebuiltSpec.gmk
>>
>>   Main changes which add make procedures and targets to build AOT-ed libraries.
>>   New environment variable TEST_OPTS_AOT_MODULES is introduced to specify list of modules to build AOT-ed libraries for.
>>   Ex: TEST_OPTS_AOT_MODULES=java.base java.logging
>>
>> - make/conf/jib-profiles.js
>>   Added Devkit installation on all platforms required to properly build AOTed libraries.
>>
>> - test/hotspot/jtreg/compiler/aot/scripts/java.base-list.txt
>> test/hotspot/jtreg/compiler/aot/scripts/jdk.internal.vm.compiler-list.txt
>>   These lists were updated based on latest jaotc errors satus.
>>
>>
>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8152988
>>  webrev: http://cr.openjdk.java.net/~epavlova//8152988/webrev.01/
> 
> In RunTests.gmk, the following line is repeated:
> 
> # Note, this could not be done during JDK build time.
> 
> 
> In the options:
> 
> $1_JAOTC_OPTS := \
> -J-Xmx4g --info \
> 
> -Xmx4g is huge, much larger than what we typically use. Is this intentional? This will risk crashing at machines with too little free memory. Will AOT fail to work with less memory?
> 
> 
> There's an extra space before the comma here:
> 
> $$(addprefix --compile-commands , $$($1_AOT_CCLIST)) \
> 
> Question (to Erik, I presume): the idea of addprefix here is that AOT_CCLIST can be empty, and this was a convenient way to just add "--compile-commands <the list file>" if it exists? I was a bit confounded at first since normally we only use it for distributing a prefix over multiple items, and I could see no way for AOT_CCLIST to ever be more than one item.
> 
> 
> About this:
> 
> # Note, ExecuteWithLog doesn't play well with with two calls in the same recipe.
> $$(info $$(JDK_IMAGE_DIR)/bin/java $$($1_JVM_OPTIONS) -XX:+PrintAOT -XX:AOTLibrary=$$@ -version)
> $$(FIXPATH) $$(JDK_IMAGE_DIR)/bin/java \
> $$($1_JVM_OPTIONS) \
> -XX:+PrintAOT -XX:AOTLibrary=$$@ -version
> 
> First, what is the purpose of this? Is it to verify that the generated AOT library works? This will result in a lot of "java -version" being printed at the time when running tests. Perhaps the output should be redirected? (Assuming that java exists with a non-zero exit value if things fail, but if it does not, I'm not sure how this would help otherwise.)
> 
> Second, there's no problem to use multiple ExecuteWithLog in the same recipe, however, the base variable needs to be different. E.g:
> 
> $$(call ExecuteWithLog, $$@_verify, \
> $$(FIXPATH) $$(JDK_IMAGE_DIR)/bin/java \
> 	    ...
> 
> 
> 
> Further,
> 
> SetupAot = $(NamedParamsMacroTemplate)
> define SetupAotBody
> ifneq ($$($1_MODULES), )
> ...
> 
> If think it would be clearer if the if-test is on the call site for SetupAot instead. Now it's unconditionally called, but does nothing if AOT is not used. I think that looks odd.
> 
> 
> And here,
> 
> - run-test-$1: clean-workdir-$1 $(TEST_PREREQS)
> + run-test-$1: clean-workdir-$1 $$($1_AOT_TARGETS)
> 
> I don't think you can just remove TEST_PREREQS like that..? (The same goes for the other run-test-$1 instance.)
> 
> 
> Finally:
> 
> + $$(addprefix -vmoption:, $$($1_AOT_OPTIONS)) \
> 
> I'm not really comfortable with this use of addprefix. I think a construct like:
> 
> ifneq ($$($1_AOT_OPTIONS), )
>    $1_JTREG_BASIC_OPTIONS += -vmoption:$$($1_AOT_OPTIONS)
> endif
> 
> would be much clearar. Ideally, I'd like to see the same construct in the SetupAotModule function as well, but that's not as important.
> 
> 
> The rest of the build changes look good. (I can't say anything about the test/**/*-list.txt files.)
> 
> /Magnus
> 
>> testing: Tested by running subset of hotspot, jdk and jck tests with TEST_OPTS_AOT_MODULES=java.base jdk.internal.vm.compiler
>>          with "make run-test" and in Mach5.
>>
>> thanks,
>> -katya
>>
>> p.s.
>>  Thanks a lot Erik for huge help with porting original patch done in old testing framework (test/TestCommon.gmk)
>>  to new one (make/RunTests*).
>>
>>
>