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

[Vladimir Kozlov]

On 10/22/18 9:36 AM, Ekaterina Pavlova wrote:
> Vladimir,
> 
> thanks a lot for the review.
> "-XX:+PrintAOT" was only passed to "java -version" which is done before the testing to verify that AOTed libraries work.

If it is only for -version then using  -XX:+PrintAOT is okay.

> It also perhaps could be useful debug info in case some tests starts fail.
> But I can change -XX:+PrintAOT to -XX:+UseAOTStrictLoading if you think it is more than enough.

-XX:+UseAOTStrictLoading is more important for tests which have different flags specified and may invalidate AOTLibrary 
configuration. We would want to catch tests which will not use AOT libraries. At least when we test these your changes.

> 
> Do you suggest to add "-Xlog:aot+class+fingerprint=trace -Xlog:aot+class+load=trace" to "java -version" only
> or to java flags used during tests execution as well?

These options are next step after -XX:+UseAOTStrictLoading. We were asked before how we can guarantee that AOTed methods 
are used by tests. One way is PrintAOT which should show published AOT methods. An other way is Xlogs flags which have 
less output but provide at least some level of information that AOTed classes are used and not skipped.

Thanks,
Vladimir

> 
> 
> -katya
> 
> On 10/19/18 10:40 AM, Vladimir Kozlov wrote:
>> I share concern about output of -XX:+PrintAOT - it prints all AOT classes and methods. For AOTed java.base the output 
>> will be almost the same for all tests (depends which are java.base classes are used).
>>
>> I would suggest instead to use -XX:+UseAOTStrictLoading which exit(1) VM if it can't load AOT library for some reason 
>> (mismatched configuration - different GCs).
>>
>> Also -Xlog:aot+class+fingerprint=trace  to trace cases of mismatching class fingerprint.
>> Also -Xlog:aot+class+load=trace -to trace AOT class loading or skipping them if they are not matching.
>>
>> And I agree to redirect this into file.
>>
>> Thanks,
>> Vladimir
>>
>> On 10/19/18 9:04 AM, Erik Joelsson wrote:
>>> Hello,
>>>
>>> I will answer the parts I'm responsible for.
>>>
>>> On 2018-10-19 00:21, Magnus Ihse Bursie wrote:
>>>>
>>>> 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.
>>>>
>>> Yes, that was the intention. I often use addprefix that way and find it convenient that it seamlessly handles 0-N 
>>> number of elements in the argument without the need for cumbersome conditionals. The extra space is needed in this 
>>> case but should perhaps be explicit using $(SPACE).
>>>>
>>>> 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.)
>>>>
>>> My guess is that they want diagnostics saved in case tests fail.
>>>> 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 \
>>>>         ...
>>>>
>>> The issue I'm referring to is that the $(info) line in ExecuteWithLog will print the commands first, then execute the 
>>> first line of the recipe. This makes the output confusing since both of those commands print (what I'm assuming is) 
>>> relevant diagnostics information. This is simply a consequence of using $(info) for printing the command line. Make 
>>> will evaluate all the recipe lines first, then execute them one by one. To be clear, it ends up like this:
>>>
>>> Executing [jaotc ...]
>>> Executing [java ...]
>>> <output from jaotc>
>>> <output from 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.
>>>>
>>> My goal was to minimize the repeated code at each call site. While developing this, I had a lot more duplication at 
>>> first, and it was a hassle to update each location every time I needed to change something. Other than that I don't 
>>> feel strongly for either construct.
>>>>
>>>> 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.)
>>>>
>>> I added the TEST_PREREQS variable to support the CDS archive generation, which was recently removed. With the way 
>>> SetupAot turned out to need separate calls from each SetupTestRun, the global TEST_PREREQS wasn't suitable. I don't 
>>> know of any current need for a general TEST_PREREQS.
>>>>
>>>> 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.
>>>>
>>> I agree.
>>>
>>> /Erik
>>>>
>>>> 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*).
>>>>>
>>>>>
>>>>
>>>
>