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

Thu Nov 1 13:46:33 UTC 2018

Hi Katya,

Sorry for the late response. :-(

On 2018-10-29 21:24, Ekaterina Pavlova wrote:
> Vladimir,
> I added "-XX:+UseAOTStrictLoading" to "java -version", see 
> make/RunTests.gmk.
> Also added "--compile-with-assertion" to jaotc flags in case "-ea" 
> flag was present in vm options
> (otherwise AOTed library will warn that it does not have java 
> assertions in compiled code).
This code:

+ ifneq ($$(findstring -ea, $$($1_VM_OPTIONS)), )

Using findstring is somewhat brittle here, since it will find substrings 
as well. Better to use filter which will match an exact word.

>
> I will add -XX:+UseAOTStrictLoading and 
> "-Xlog:aot+class+fingerprint=trace -Xlog:aot+class+load=trace"
> directly to AOT testing tasks in mach5 jobs.
>
> Magnus,
> I changed
>  "$$(addprefix -vmoption:, $$($1_AOT_OPTIONS))"
> to
> +  ifneq ($$($1_AOT_OPTIONS), )
> +    $1_JTREG_BASIC_OPTIONS += -vmoptions:"$$($1_AOT_OPTIONS)"
> +  endif
>
> Please review updated webrev:
>  http://cr.openjdk.java.net/~epavlova//8152988/webrev.01/

Some fallout from my and Erik's discussion still needs implementing in 
RunTests.gmk:

$$(addprefix --compile-commands , $$($1_AOT_CCLIST)) \

Please rewrite as:

$$(addprefix --compile-commands$(SPACE), $$($1_AOT_CCLIST)) \

I'm still not happy with the always printed "java -version". :( Vladimir 
agreed that it would be reasonable to redirect this into a file. This 
can be done by e.g.

+ $$(call ExecuteWithLog, $$@.check, \
+ $$(FIXPATH) $$(JDK_IMAGE_DIR)/bin/java \
+ $$($1_VM_OPTIONS) -XX:+PrintAOT -XX:+UseAOTStrictLoading 
-XX:AOTLibrary=$$@ -version \
+ > $$@.verify-aot
+ )

This will redirect the output to $($1_AOT_LIB).verify-aot in 
test-support/aot.

I still would like to see the if statement around SetupAot moved out, 
like this:

+ ifneq ($$(GTEST_AOT_MODULES), ) + $$(eval $$(call SetupAot, $1, \
+ MODULES := $$(GTEST_AOT_MODULES), \
+ VM_OPTIONS := $$(GTEST_VM_OPTIONS) $$(GTEST_JAVA_OPTIONS), \
+ )) + endif

and similarly for JTREG. This is coupled with removing the "ifneq 
($$($1_MODULES), )" in SetupAotBody (and do not forget to un-indent two 
spaces).

/Magnus

>
> thanks,
> -katya
>
>
> On 10/23/18 4:40 PM, Vladimir Kozlov wrote:
>> 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*).
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20181101/03751fed/attachment-0001.html>