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.pavl...@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*).
