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