RFR(M) 8152988: [AOT] Update test batch definitions to include aot-ed java.base module mode into hs-comp testing
vladimir.kozlov at oracle.com
Fri Oct 19 17:40:19 UTC 2018
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.
On 10/19/18 9:04 AM, Erik Joelsson wrote:
> 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>
>> 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.
>> + $$(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)
>> 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.
>> The rest of the build changes look good. (I can't say anything about the test/**/*-list.txt files.)
>>> testing: Tested by running subset of hotspot, jdk and jck tests with TEST_OPTS_AOT_MODULES=java.base
>>> with "make run-test" and in Mach5.
>>> 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 hotspot-compiler-dev