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