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

Magnus Ihse Bursie magnus.ihse.bursie at oracle.com
Fri Nov 2 08:19:25 UTC 2018


On 2018-11-02 05:32, Ekaterina Pavlova wrote:
> yes, it is at the same location:
>  http://cr.openjdk.java.net/~epavlova//8152988/webrev.01/
I see.

In the future, please do not update webrevs "in place" (except perhaps 
for minor changes like a typo in a comment). This invalidates the 
history of the webrev, and makes it impossible to follow the discussion 
in the mailing list. Instead, create a new version of the webrev each 
time (webrev.02, ...).

Looks good now. Thanks for hanging in there, even though it has taken 
quite some time!

I noticed a single minor thing, there's a double space here:
+    $1_JAOTC_OPTS +=  --compile-with-assertions

If you want to, you can remove it. You do not need any more webrevs for 
that.

/Magnus

>
> -katya
>
> On 11/1/18 3:15 PM, Magnus Ihse Bursie wrote:
>> Do you have an updated webrev?
>>
>> /Magnus
>>
>>> 1 nov. 2018 kl. 22:28 skrev Ekaterina Pavlova 
>>> <ekaterina.pavlova at oracle.com>:
>>>
>>> Magnus,
>>>
>>> thanks for the review.
>>> I fixed everything you pointed to.
>>>
>>> thanks,
>>> -katya
>>>
>>>> On 11/1/18 6:46 AM, Magnus Ihse Bursie wrote:
>>>> 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*).
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>
>



More information about the build-dev mailing list