Re: RFR: 8255616: Disable AOT and Graal in Oracle OpenJDK
On Fri, 30 Oct 2020 17:52:09 GMT, Igor Ignatyev wrote: >> We shipped Ahead-of-Time compilation (the jaotc tool) in JDK 9, as an >> experimental feature. We shipped Graal as an experimental JIT compiler in >> JDK 10. We haven't seen much use of these features, and the effort required >> to support and enhance them is significant. We therefore intend to disable >> these features in Oracle builds as of JDK 16. >> >> We'll leave the sources for these features in the repository, in case any >> one else is interested in building them. But we will not update or test them. >> >> We'll continue to build and ship JVMCI as an experimental feature in Oracle >> builds. >> >> Tested changes in all tiers. >> >> I verified that with these changes I still able to build Graal in open repo >> and run graalunit testing: >> >> `open$ bash test/hotspot/jtreg/compiler/graalunit/downloadLibs.sh >> /mydir/graalunit_lib/` >> `open$ bash configure --with-debug-level=fastdebug >> --with-graalunit-lib=/mydir/graalunit_lib/ --with-jtreg=/mydir/jtreg` >> `open$ make jdk-image` >> `open$ make test-image` >> `open$ make run-test TEST=compiler/graalunit/HotspotTest.java` > > LGTM Looks good. - PR: https://git.openjdk.java.net/jdk/pull/960
Re: RFR: 8223347: Integration of Vector API (Incubator) [v4]
On Wed, 14 Oct 2020 00:34:04 GMT, Sandhya Viswanathan wrote: >> There are several gc tests crashed in panama-vector tier3 testing which >> seems are not observed in openjdk repo. >> The crashes look like: >> # assert(oopDesc::is_oop(obj)) failed: not an oop: 0xfff1 >> # >> # JRE version: Java(TM) SE Runtime Environment (16.0+3) (fastdebug build >> 16-panama+3-216) >> # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 16-panama+3-216, >> mixed mode, sharing, tiered, compressed oops, >> g1 gc, linux-amd64) # Problematic frame: >> # V [libjvm.so+0xd8ef94] HandleArea::allocate_handle(oop)+0x144 >> >> and the issue is actually tracked by JDK-8233199. >> >> This issue needs to be at least analyzed before integrating Vector API. > > @katyapav Is the failure observed on vector-unstable branch of panama-vector? > The code in this pull request is from vector-unstable branch. > The bug report https://bugs.openjdk.java.net/browse/JDK-8233199 refers to > repo-valhalla and not > panama-vector:vector-unstable. @PaulSandoz is doing final testing of the pull > request today before integration tomorrow > hopefully. @sviswa7 you are right, the failure is observed on vector-unstable branch of panama-vector. I referred to JDK-8233199 because it seems both panama-vector and valhalla-repo have the same issue/crash. @PaulSandoz also mentioned that panama-vector was not in sync with master and this is perhaps the issue is in vector-unstable. He said that he tested the PR separately and didn't observe this issue in the PR. - PR: https://git.openjdk.java.net/jdk/pull/367
Re: RFR: 8223347: Integration of Vector API (Incubator) [v4]
On Mon, 12 Oct 2020 12:56:10 GMT, Erik Joelsson wrote: >> Paul Sandoz has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now >> contains ten commits: >> - Merge master >> - Fix related to merge >> - HotspotIntrinsicCandidate to IntrinsicCandidate >> - Merge master >> - Fix permissions >> - Fix permissions >> - Merge master >> - Vector API new files >> - Integration of Vector API (Incubator) > > Build changes look good. There are several gc tests crashed in panama-vector tier3 testing which seems are not observed in openjdk repo. The crashes look like: # assert(oopDesc::is_oop(obj)) failed: not an oop: 0xfff1 # # JRE version: Java(TM) SE Runtime Environment (16.0+3) (fastdebug build 16-panama+3-216) # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 16-panama+3-216, mixed mode, sharing, tiered, compressed oops, g1 gc, linux-amd64) # Problematic frame: # V [libjvm.so+0xd8ef94] HandleArea::allocate_handle(oop)+0x144 and the issue is actually tracked by JDK-8233199. This issue needs to be at least analyzed before integrating Vector API. - PR: https://git.openjdk.java.net/jdk/pull/367
Re: RFR(T/XS) 8231145: [Graal] org.graalvm.compiler.debug.test.DebugContextTest fails because DebugContextTest.testLogging.input is not available
Erik, this way is definitely much better, thanks! I have regenerated webrev and retested: http://cr.openjdk.java.net/~epavlova//8231145/webrev.00/index.html -katya On 9/18/19 8:18 AM, Erik Joelsson wrote: Hello Katya, The usual pattern for including a non class file from the source into the jar (typically a resource file) is to add it to the COPY parameter of SetupJavaCompilation, and then to the SUFFIXES of SetupJarArchive. This way you don't need to add a source dir to the input of SetupJarArchive, which is a bit weird. So in this case, you can add COPY := .input to BUILD_VM_COMPILER_TESTS and keep the SUFFIXES you already added, and skip the addition to SRCS. /Erik On 2019-09-17 15:45, Ekaterina Pavlova wrote: Hi, please review the following change which fixes org.graalvm.compiler.debug.test.DebugContextTest. The test fails because it tries to read DebugContextTest.testLogging.input file which is not available at runtime. The fix copies testLogging.input file into jdk.vm.compiler.tests.jar. JBS: https://bugs.openjdk.java.net/browse/JDK-8231145 webrev: http://cr.openjdk.java.net/~epavlova//8231145/webrev.00/index.html testing: run compiler/graalunit/DebugTest.java thanks, -katya
Re: RFR(T/XS) 8231145: [Graal] org.graalvm.compiler.debug.test.DebugContextTest fails because DebugContextTest.testLogging.input is not available
Igor, Magnus, thank you for reviews. -katya On 9/18/19 2:30 AM, Magnus Ihse Bursie wrote: On 2019-09-18 00:45, Ekaterina Pavlova wrote: Hi, please review the following change which fixes org.graalvm.compiler.debug.test.DebugContextTest. The test fails because it tries to read DebugContextTest.testLogging.input file which is not available at runtime. The fix copies testLogging.input file into jdk.vm.compiler.tests.jar. JBS: https://bugs.openjdk.java.net/browse/JDK-8231145 webrev: http://cr.openjdk.java.net/~epavlova//8231145/webrev.00/index.html Looks good to me. /Magnus testing: run compiler/graalunit/DebugTest.java thanks, -katya
RFR(T/XS) 8231145: [Graal] org.graalvm.compiler.debug.test.DebugContextTest fails because DebugContextTest.testLogging.input is not available
Hi, please review the following change which fixes org.graalvm.compiler.debug.test.DebugContextTest. The test fails because it tries to read DebugContextTest.testLogging.input file which is not available at runtime. The fix copies testLogging.input file into jdk.vm.compiler.tests.jar. JBS: https://bugs.openjdk.java.net/browse/JDK-8231145 webrev: http://cr.openjdk.java.net/~epavlova//8231145/webrev.00/index.html testing: run compiler/graalunit/DebugTest.java thanks, -katya
Re: RFR (XXS) 8214557: Filter out VM flags which don't affect AOT code generation
Thanks Erik for prompt review. -katya On 11/30/18 2:54 PM, Erik Joelsson wrote: Looks ok. /Erik On 2018-11-30 14:47, Ekaterina Pavlova wrote: Hi, AOT requires that the same java runtime configuration should be used during AOT compilation and execution. This implies the same VM flags used by java and jaotc. However some flags could dramatically slow down jaotc while don't affecting AOT code generation at all. This fix filters out couple of such flags. Please review. JBS: https://bugs.openjdk.java.net/browse/JDK-8214557 webrev: http://cr.openjdk.java.net/~epavlova//8214557/webrev.00/index.html testing: Tested by running with AOTed modules. thanks, -katya
RFR (XXS) 8214557: Filter out VM flags which don't affect AOT code generation
Hi, AOT requires that the same java runtime configuration should be used during AOT compilation and execution. This implies the same VM flags used by java and jaotc. However some flags could dramatically slow down jaotc while don't affecting AOT code generation at all. This fix filters out couple of such flags. Please review. JBS: https://bugs.openjdk.java.net/browse/JDK-8214557 webrev: http://cr.openjdk.java.net/~epavlova//8214557/webrev.00/index.html testing: Tested by running with AOTed modules. thanks, -katya
Re: RFR(M) 8152988: [AOT] Update test batch definitions to include aot-ed java.base module mode into hs-comp testing
yes, it is at the same location: http://cr.openjdk.java.net/~epavlova//8152988/webrev.01/ -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 : 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
Re: RFR(M) 8152988: [AOT] Update test batch definitions to include aot-ed java.base module mode into hs-comp testing
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 i
Re: RFR(M) 8152988: [AOT] Update test batch definitions to include aot-ed java.base module mode into hs-comp testing
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). 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/ 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 " 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 \
Re: RFR(XXS) 8212877: Restore JTREG_VERBOSE value for mach5 testing
Thanks David. -katya On 10/23/18 3:04 PM, David Holmes wrote: LGTM. Thanks, David On 24/10/2018 6:33 AM, Ekaterina Pavlova wrote: Hi All, Please review this extra small change which restores JTREG_VERBOSE value for mach5 testing. Default JTREG_VERBOSE value was changed from "fail,error,time" to "fail,error,summary" after switching to run-test framework (JDK-8212028). Having compact tests output is good for local users while mach5 testing will benefits from providing more test execution information. Specially having test time execution data helps to analyze any tests/jobs slowness. JBS: https://bugs.openjdk.java.net/browse/JDK-8212877 webrev: http://cr.openjdk.java.net/~epavlova//8212877/webrev.00/index.html testing: tested by launching jobs in Mach5 thanks, -katya
RFR(XXS) 8212877: Restore JTREG_VERBOSE value for mach5 testing
Hi All, Please review this extra small change which restores JTREG_VERBOSE value for mach5 testing. Default JTREG_VERBOSE value was changed from "fail,error,time" to "fail,error,summary" after switching to run-test framework (JDK-8212028). Having compact tests output is good for local users while mach5 testing will benefits from providing more test execution information. Specially having test time execution data helps to analyze any tests/jobs slowness. JBS: https://bugs.openjdk.java.net/browse/JDK-8212877 webrev: http://cr.openjdk.java.net/~epavlova//8212877/webrev.00/index.html testing: tested by launching jobs in Mach5 thanks, -katya
Re: RFR(M) 8152988: [AOT] Update test batch definitions to include aot-ed java.base module mode into hs-comp testing
Magnus, thanks a lot for your review. Regarding "-J-Xmx4g --info". This is done intentionally. AOT requires more heap usage and could fail with OOM otherwise. I specify "mem.total" in AOT jobs definitions so AOT testing is done only on machines which has at least 4gb. -katya On 10/19/18 12:21 AM, Magnus Ihse Bursie wrote: On 2018-10-18 08:29, Ekaterina Pavlova wrote: Hi All, Please review the changes which add support to build AOTed jdk modules and then use them during the testing. Note, building of AOTed libraries needs to be done at the same machine the testing is going to be started. So, this could not be done at jdk build time. Updated files: - make/RunTests.gmk, make/RunTestsPrebuilt.gmk, make/RunTestsPrebuiltSpec.gmk Main changes which add make procedures and targets to build AOT-ed libraries. New environment variable TEST_OPTS_AOT_MODULES is introduced to specify list of modules to build AOT-ed libraries for. Ex: TEST_OPTS_AOT_MODULES=java.base java.logging - make/conf/jib-profiles.js Added Devkit installation on all platforms required to properly build AOTed libraries. - test/hotspot/jtreg/compiler/aot/scripts/java.base-list.txt test/hotspot/jtreg/compiler/aot/scripts/jdk.internal.vm.compiler-list.txt These lists were updated based on latest jaotc errors satus. JBS: https://bugs.openjdk.java.net/browse/JDK-8152988 webrev: http://cr.openjdk.java.net/~epavlova//8152988/webrev.01/ 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 " 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. 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.) 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 \ ... 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. 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.) 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. 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*).
Re: RFR(M) 8152988: [AOT] Update test batch definitions to include aot-ed java.base module mode into hs-comp testing
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. 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. 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? -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 " 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 ...] 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
RFR(M) 8152988: [AOT] Update test batch definitions to include aot-ed java.base module mode into hs-comp testing
Hi All, Please review the changes which add support to build AOTed jdk modules and then use them during the testing. Note, building of AOTed libraries needs to be done at the same machine the testing is going to be started. So, this could not be done at jdk build time. Updated files: - make/RunTests.gmk, make/RunTestsPrebuilt.gmk, make/RunTestsPrebuiltSpec.gmk Main changes which add make procedures and targets to build AOT-ed libraries. New environment variable TEST_OPTS_AOT_MODULES is introduced to specify list of modules to build AOT-ed libraries for. Ex: TEST_OPTS_AOT_MODULES=java.base java.logging - make/conf/jib-profiles.js Added Devkit installation on all platforms required to properly build AOTed libraries. - test/hotspot/jtreg/compiler/aot/scripts/java.base-list.txt test/hotspot/jtreg/compiler/aot/scripts/jdk.internal.vm.compiler-list.txt These lists were updated based on latest jaotc errors satus. JBS: https://bugs.openjdk.java.net/browse/JDK-8152988 webrev: http://cr.openjdk.java.net/~epavlova//8152988/webrev.01/ 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*).
Re: RFR(S) 8205078: [Graal] org.graalvm.compiler.core.test.VerifyDebugUsageTest fails with "Expected exception: VerificationError"
Doug, Vladimir, thanks for your reviews. -katya On 8/7/18 9:10 AM, Vladimir Kozlov wrote: Okay. Changes are good then. Vladimir On 8/6/18 11:40 PM, Doug Simon wrote: On 7 Aug 2018, at 08:07, Ekaterina Pavlova wrote: Vladimir, thanks for prompt review. I can revert the changes and add -XDstringConcat=inline to JAVAC flags for all tests. I did it only for org/graalvm/compiler/core/test/VerifyDebugUsageTest.java because Doug was not sure if this should be applied to all tests. Here what he said: "I can't say for sure since we've only ever compiled the tests with JDK 8 (or --release 8). I would only apply it to this test for now. Doug, what is your opinion on this? Still the same. Tests should fail fast if they cannot handle Indy strings so I think dealing with it on a case-by-case basis is the right approach. -Doug On 8/6/18 2:17 PM, Vladimir Kozlov wrote: Can we switch off Indy strings concat for all Graal unit tests? Based on what Doug said: "We don't support the indy string pattern in these tests". Note, Graal itself (and JVMCI) are compiled without Indy strings concat: http://hg.openjdk.java.net/jdk/jdk/file/c23f1e4910f3/make/CompileJavaModules.gmk#l425 Thanks, Vladimir On 8/6/18 1:42 PM, Ekaterina Pavlova wrote: Hi All, please review the change which introduces additional make target in JtregGraalUnit.gmk to compile org/graalvm/compiler/core/test/VerifyDebugUsageTest.java with -XDstringConcat=inline. Otherwise the test fails. Also moved building of jdk.vm.compiler.replacements.verifier.jar into separate target. JBS: https://bugs.openjdk.java.net/browse/JDK-8205078 webrev: http://cr.openjdk.java.net/~epavlova//8205078/webrev.00/index.html testing: tested by building on all platforms and running graal unit tests on Graal supported platforms. thanks, -katya
Re: RFR(S) 8205078: [Graal] org.graalvm.compiler.core.test.VerifyDebugUsageTest fails with "Expected exception: VerificationError"
Vladimir, thanks for prompt review. I can revert the changes and add -XDstringConcat=inline to JAVAC flags for all tests. I did it only for org/graalvm/compiler/core/test/VerifyDebugUsageTest.java because Doug was not sure if this should be applied to all tests. Here what he said: "I can't say for sure since we've only ever compiled the tests with JDK 8 (or --release 8). I would only apply it to this test for now. Doug, what is your opinion on this? thanks, -katya On 8/6/18 2:17 PM, Vladimir Kozlov wrote: Can we switch off Indy strings concat for all Graal unit tests? Based on what Doug said: "We don't support the indy string pattern in these tests". Note, Graal itself (and JVMCI) are compiled without Indy strings concat: http://hg.openjdk.java.net/jdk/jdk/file/c23f1e4910f3/make/CompileJavaModules.gmk#l425 Thanks, Vladimir On 8/6/18 1:42 PM, Ekaterina Pavlova wrote: Hi All, please review the change which introduces additional make target in JtregGraalUnit.gmk to compile org/graalvm/compiler/core/test/VerifyDebugUsageTest.java with -XDstringConcat=inline. Otherwise the test fails. Also moved building of jdk.vm.compiler.replacements.verifier.jar into separate target. JBS: https://bugs.openjdk.java.net/browse/JDK-8205078 webrev: http://cr.openjdk.java.net/~epavlova//8205078/webrev.00/index.html testing: tested by building on all platforms and running graal unit tests on Graal supported platforms. thanks, -katya
RFR(S) 8205078: [Graal] org.graalvm.compiler.core.test.VerifyDebugUsageTest fails with "Expected exception: VerificationError"
Hi All, please review the change which introduces additional make target in JtregGraalUnit.gmk to compile org/graalvm/compiler/core/test/VerifyDebugUsageTest.java with -XDstringConcat=inline. Otherwise the test fails. Also moved building of jdk.vm.compiler.replacements.verifier.jar into separate target. JBS: https://bugs.openjdk.java.net/browse/JDK-8205078 webrev: http://cr.openjdk.java.net/~epavlova//8205078/webrev.00/index.html testing: tested by building on all platforms and running graal unit tests on Graal supported platforms. thanks, -katya
Re: RFR(XXS): 8206113: Troubles configuring graal tests
On 7/2/18 2:17 PM, David Holmes wrote: Hi Katya, On 3/07/2018 5:10 AM, Ekaterina Pavlova wrote: Hi All, please review this extra small change which makes make more silent by removing info message in case graal unit tests are not built. Happy to see the build-time warning go, but I thought it was going to be replaced by some kind of configure-time warning? Sure, I will file new rfe for this. I just wanted to remove built-time warning quicker so it will stop annoying. thanks, -katya Thanks, David JBS: https://bugs.openjdk.java.net/browse/JDK-8206113 webrev: http://cr.openjdk.java.net/~epavlova//8206113/webrev.00/index.html thanks, -katya p.s. Igor Ignatyev volunteered to sponsor this change.
RFR(XXS): 8206113: Troubles configuring graal tests
Hi All, please review this extra small change which makes make more silent by removing info message in case graal unit tests are not built. JBS: https://bugs.openjdk.java.net/browse/JDK-8206113 webrev: http://cr.openjdk.java.net/~epavlova//8206113/webrev.00/index.html thanks, -katya p.s. Igor Ignatyev volunteered to sponsor this change.
Re: RFR(XXS) : 8206088 : 8205207 broke builds
Good, thanks for quick response and webrev! -katya On 6/28/18 9:42 PM, Igor Ignatyev wrote: http://cr.openjdk.java.net/~iignatyev//8206088/webrev.00/index.html 1 line changed: 0 ins; 0 del; 1 mod; Hi all, could you please review this one liner fix? webrev: http://cr.openjdk.java.net/~iignatyev//8206088/webrev.00/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8206088 testing: make build-test-hotspot-jtreg-graal w/ empty GRAALUNIT_LIB Thanks, -- Igor
Re: RFR(M): 8205207: Port Graal unit tests under jtreg
Thanks Erik! -katya On 6/27/18 8:36 AM, Erik Joelsson wrote: On 2018-06-27 00:29, Ekaterina Pavlova wrote: well, INCLUDE_GRAAL is not defined at the time we run tests. I can try to guard by something like ifeq ($(OPENJDK_TARGET_OS)-$(OPENJDK_TARGET_CPU), $(filter $(OPENJDK_TARGET_OS)-$(OPENJDK_TARGET_CPU),linux-x64 macosx-x64 windows-x64)) but I am not sure we should proceed this way. It looks too much complicated. It is safe to pass -e:TEST_IMAGE_GRAAL_DIR to jtreg even it will not be used. We also do similar way for example for -e:JDK8_HOME I agree, no need to guard the addition of the env variable pass through. I have uploaded new webrev at the same location: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html Looks good to me now. (Magnus is on vacation so you probably don't want to wait for him to respond) /Erik thanks for reviewing, -katya /Magnus Erik, thanks again for your detailed reviews! -katya On 6/22/18 2:38 PM, Erik Joelsson wrote: Hello Katya, This looks much better, thanks! A few suggestions still: Main.gmk: instead of repeating the assignment in both if and else block: ifeq ($(INCLUDE_GRAAL), true) JVM_TEST_IMAGE_TARGETS += test-image-hotspot-jtreg-graal endif I think it's fine to do that without the ?= because an alternative JVM implementation is unlikely to be using graal anyway. In JtregGraalUnit.gmk: Some minor style nits: 54: align )) with first eval line as you have done with the other eval calls 118: Since 117 is a one line parameter, please move comma to 117 133: Same as for 118 130-132: Please indent 4 spaces for continuation /Erik On 2018-06-22 14:16, Ekaterina Pavlova wrote: Erik, Doug, thank you a lot for your reviews and advises. I fixed everything what Erik has pointed out, please see my answers inlined. As about moving more staff in 'updategraalinopenjdk' can we consider this as next step? I am not quite familiar with 'updategraalinopenjdk' and didn't contribute into Graal ws yet, so I will prefer to do this improvement/refactoring as part of separate JDK issue. New webrev is here: webrev: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8205207 testing: tested by building and running graal unit tests On 6/19/18 2:00 PM, Erik Joelsson wrote: Hello, Please always include build-dev when reviewing build changes. In general this looks pretty good but there are some details that need fixing. (Aside from this particular review, I must state my reservation against all the special treatment graal is enjoying from a source and test layout and build perspective. My understanding here is that someone will have to regularly update the wrapper jtreg tests manually using the script. This in addition to having to implement this very convoluted redirection logic because the tests are in the wrong place. Wouldn't it make a lot more sense to put the complicated logic in the import procedure for graal source so that it would fit nicely into the OpenJDK source/build/test model, instead of adding more and more complicated workarounds in the OpenJDK build and test procedures?) On 2018-06-18 22:26, Ekaterina Pavlova wrote: Hi All, please review porting of Graal unit tests under jtreg. The main idea of this porting is to keep Graal unit tests as is (located in src/jdk.internal.vm.compiler/share/classes/*.test) and run them similar way they are run in Graal project. To achieve this test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java helper class has been written which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to run specified set of Graal unit tests. The groups of Graal unit tests to run are defined in auto-generated test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests. New make/test/JtregGraalUnit.gmk is used to build Graal unit tests into jdk.vm.compiler.tests.jar and then install it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/. GRAALUNIT_LIB is never defined (in the open). Since this is used in open makefiles, we need an open configure option to set it. ok, I created open/make/autoconf/lib-tests.m4 and did put Graal related staff there. To make things clearer, I would rename LIB_DIR to something like LIB_OUTPUTDIR to signal that this is a location to put files in, rather than read them from. ok, renamed FLATTEN has no effect in the SetupCopyFiles call since all the jar files found by wildcard can only be in one directory anyway. ok, removed BUILDTOOLS_OUTPUTDIR is meant for tools used during the build. Tests classes and jars should be built into $(SUPPORT_OUTPUTDIR)/test/..> Please create a suitable sub directory there for the output from this makefile. ok, introduced COMPILE_OUTPUTDIR := $(SUPPORT_OUTPUTDIR)/test/graalunit to be used to build graal unit test classes. The all and test-image targets are never called so no need to declare them. T
Re: RFR(M): 8205207: Port Graal unit tests under jtreg
On 6/26/18 9:08 AM, Ekaterina Pavlova wrote: Hello Magnus, On 6/26/18 12:50 AM, Magnus Ihse Bursie wrote: 23 juni 2018 kl. 00:22 skrev Ekaterina Pavlova mailto:ekaterina.pavl...@oracle.com>>: Fixed and regenerated webrev at the same location: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html Better, but not done yet. In JtregGraalUnit.gmk: line 52-53 should be on a single line. will fix done The SRC list for BUILD_VM_COMPILER_TESTS looks insane, but the root cause here is the weird layout on disk. But the .test part really surprises me, I thought we had a clear rule to have no test source in src?! These are not jtreg tests, these are graalunit tests which are coming as part of Graal port (from Graal ws). We can't change this layout right now. In lib-tests.m4: Copy paste error: AC_MSG_ERROR([You must specify the path to tonga jar]) ops, will fix done The addition in RunTests.gmk and TestCommon.gmk should be guarded by: ifeq ($(INCLUDE_GRAAL), true) ok, will look well, INCLUDE_GRAAL is not defined at the time we run tests. I can try to guard by something like ifeq ($(OPENJDK_TARGET_OS)-$(OPENJDK_TARGET_CPU), $(filter $(OPENJDK_TARGET_OS)-$(OPENJDK_TARGET_CPU),linux-x64 macosx-x64 windows-x64)) but I am not sure we should proceed this way. It looks too much complicated. It is safe to pass -e:TEST_IMAGE_GRAAL_DIR to jtreg even it will not be used. We also do similar way for example for -e:JDK8_HOME I have uploaded new webrev at the same location: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html thanks for reviewing, -katya /Magnus Erik, thanks again for your detailed reviews! -katya On 6/22/18 2:38 PM, Erik Joelsson wrote: Hello Katya, This looks much better, thanks! A few suggestions still: Main.gmk: instead of repeating the assignment in both if and else block: ifeq ($(INCLUDE_GRAAL), true) JVM_TEST_IMAGE_TARGETS += test-image-hotspot-jtreg-graal endif I think it's fine to do that without the ?= because an alternative JVM implementation is unlikely to be using graal anyway. In JtregGraalUnit.gmk: Some minor style nits: 54: align )) with first eval line as you have done with the other eval calls 118: Since 117 is a one line parameter, please move comma to 117 133: Same as for 118 130-132: Please indent 4 spaces for continuation /Erik On 2018-06-22 14:16, Ekaterina Pavlova wrote: Erik, Doug, thank you a lot for your reviews and advises. I fixed everything what Erik has pointed out, please see my answers inlined. As about moving more staff in 'updategraalinopenjdk' can we consider this as next step? I am not quite familiar with 'updategraalinopenjdk' and didn't contribute into Graal ws yet, so I will prefer to do this improvement/refactoring as part of separate JDK issue. New webrev is here: webrev: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8205207 testing: tested by building and running graal unit tests On 6/19/18 2:00 PM, Erik Joelsson wrote: Hello, Please always include build-dev when reviewing build changes. In general this looks pretty good but there are some details that need fixing. (Aside from this particular review, I must state my reservation against all the special treatment graal is enjoying from a source and test layout and build perspective. My understanding here is that someone will have to regularly update the wrapper jtreg tests manually using the script. This in addition to having to implement this very convoluted redirection logic because the tests are in the wrong place. Wouldn't it make a lot more sense to put the complicated logic in the import procedure for graal source so that it would fit nicely into the OpenJDK source/build/test model, instead of adding more and more complicated workarounds in the OpenJDK build and test procedures?) On 2018-06-18 22:26, Ekaterina Pavlova wrote: Hi All, please review porting of Graal unit tests under jtreg. The main idea of this porting is to keep Graal unit tests as is (located in src/jdk.internal.vm.compiler/share/classes/*.test) and run them similar way they are run in Graal project. To achieve this test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java helper class has been written which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to run specified set of Graal unit tests. The groups of Graal unit tests to run are defined in auto-generated test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests. New make/test/JtregGraalUnit.gmk is used to build Graal unit tests into jdk.vm.compiler.tests.jar and then install it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/. GRAALUNIT_LIB is never defined (in the open). Since this is used in open makefiles, we need an open configure option to set it. ok, I created open/make/autoconf/lib-tests.m4 and did put Graal related staff there. To
Re: RFR(M): 8205207: Port Graal unit tests under jtreg
Hello Magnus, On 6/26/18 12:50 AM, Magnus Ihse Bursie wrote: 23 juni 2018 kl. 00:22 skrev Ekaterina Pavlova mailto:ekaterina.pavl...@oracle.com>>: Fixed and regenerated webrev at the same location: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html Better, but not done yet. In JtregGraalUnit.gmk: line 52-53 should be on a single line. will fix The SRC list for BUILD_VM_COMPILER_TESTS looks insane, but the root cause here is the weird layout on disk. But the .test part really surprises me, I thought we had a clear rule to have no test source in src?! These are not jtreg tests, these are graalunit tests which are coming as part of Graal port (from Graal ws). We can't change this layout right now. In lib-tests.m4: Copy paste error: AC_MSG_ERROR([You must specify the path to tonga jar]) ops, will fix The addition in RunTests.gmk and TestCommon.gmk should be guarded by: ifeq ($(INCLUDE_GRAAL), true) ok, will look /Magnus Erik, thanks again for your detailed reviews! -katya On 6/22/18 2:38 PM, Erik Joelsson wrote: Hello Katya, This looks much better, thanks! A few suggestions still: Main.gmk: instead of repeating the assignment in both if and else block: ifeq ($(INCLUDE_GRAAL), true) JVM_TEST_IMAGE_TARGETS += test-image-hotspot-jtreg-graal endif I think it's fine to do that without the ?= because an alternative JVM implementation is unlikely to be using graal anyway. In JtregGraalUnit.gmk: Some minor style nits: 54: align )) with first eval line as you have done with the other eval calls 118: Since 117 is a one line parameter, please move comma to 117 133: Same as for 118 130-132: Please indent 4 spaces for continuation /Erik On 2018-06-22 14:16, Ekaterina Pavlova wrote: Erik, Doug, thank you a lot for your reviews and advises. I fixed everything what Erik has pointed out, please see my answers inlined. As about moving more staff in 'updategraalinopenjdk' can we consider this as next step? I am not quite familiar with 'updategraalinopenjdk' and didn't contribute into Graal ws yet, so I will prefer to do this improvement/refactoring as part of separate JDK issue. New webrev is here: webrev: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8205207 testing: tested by building and running graal unit tests On 6/19/18 2:00 PM, Erik Joelsson wrote: Hello, Please always include build-dev when reviewing build changes. In general this looks pretty good but there are some details that need fixing. (Aside from this particular review, I must state my reservation against all the special treatment graal is enjoying from a source and test layout and build perspective. My understanding here is that someone will have to regularly update the wrapper jtreg tests manually using the script. This in addition to having to implement this very convoluted redirection logic because the tests are in the wrong place. Wouldn't it make a lot more sense to put the complicated logic in the import procedure for graal source so that it would fit nicely into the OpenJDK source/build/test model, instead of adding more and more complicated workarounds in the OpenJDK build and test procedures?) On 2018-06-18 22:26, Ekaterina Pavlova wrote: Hi All, please review porting of Graal unit tests under jtreg. The main idea of this porting is to keep Graal unit tests as is (located in src/jdk.internal.vm.compiler/share/classes/*.test) and run them similar way they are run in Graal project. To achieve this test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java helper class has been written which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to run specified set of Graal unit tests. The groups of Graal unit tests to run are defined in auto-generated test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests. New make/test/JtregGraalUnit.gmk is used to build Graal unit tests into jdk.vm.compiler.tests.jar and then install it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/. GRAALUNIT_LIB is never defined (in the open). Since this is used in open makefiles, we need an open configure option to set it. ok, I created open/make/autoconf/lib-tests.m4 and did put Graal related staff there. To make things clearer, I would rename LIB_DIR to something like LIB_OUTPUTDIR to signal that this is a location to put files in, rather than read them from. ok, renamed FLATTEN has no effect in the SetupCopyFiles call since all the jar files found by wildcard can only be in one directory anyway. ok, removed BUILDTOOLS_OUTPUTDIR is meant for tools used during the build. Tests classes and jars should be built into $(SUPPORT_OUTPUTDIR)/test/..> Please create a suitable sub directory there for the output from this makefile. ok, introduced COMPILE_OUTPUTDIR := $(SUPPORT_OUTPUTDIR)/test/graalunit to be used to build graal unit test classes.
Re: RFR(M): 8205207: Port Graal unit tests under jtreg
Thanks Erik! -katya On 6/25/18 8:52 AM, Erik Joelsson wrote: Looks ok to me. /Erik On 2018-06-22 15:22, Ekaterina Pavlova wrote: Fixed and regenerated webrev at the same location: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html Erik, thanks again for your detailed reviews! -katya On 6/22/18 2:38 PM, Erik Joelsson wrote: Hello Katya, This looks much better, thanks! A few suggestions still: Main.gmk: instead of repeating the assignment in both if and else block: ifeq ($(INCLUDE_GRAAL), true) JVM_TEST_IMAGE_TARGETS += test-image-hotspot-jtreg-graal endif I think it's fine to do that without the ?= because an alternative JVM implementation is unlikely to be using graal anyway. In JtregGraalUnit.gmk: Some minor style nits: 54: align )) with first eval line as you have done with the other eval calls 118: Since 117 is a one line parameter, please move comma to 117 133: Same as for 118 130-132: Please indent 4 spaces for continuation /Erik On 2018-06-22 14:16, Ekaterina Pavlova wrote: Erik, Doug, thank you a lot for your reviews and advises. I fixed everything what Erik has pointed out, please see my answers inlined. As about moving more staff in 'updategraalinopenjdk' can we consider this as next step? I am not quite familiar with 'updategraalinopenjdk' and didn't contribute into Graal ws yet, so I will prefer to do this improvement/refactoring as part of separate JDK issue. New webrev is here: webrev: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8205207 testing: tested by building and running graal unit tests On 6/19/18 2:00 PM, Erik Joelsson wrote: Hello, Please always include build-dev when reviewing build changes. In general this looks pretty good but there are some details that need fixing. (Aside from this particular review, I must state my reservation against all the special treatment graal is enjoying from a source and test layout and build perspective. My understanding here is that someone will have to regularly update the wrapper jtreg tests manually using the script. This in addition to having to implement this very convoluted redirection logic because the tests are in the wrong place. Wouldn't it make a lot more sense to put the complicated logic in the import procedure for graal source so that it would fit nicely into the OpenJDK source/build/test model, instead of adding more and more complicated workarounds in the OpenJDK build and test procedures?) On 2018-06-18 22:26, Ekaterina Pavlova wrote: Hi All, please review porting of Graal unit tests under jtreg. The main idea of this porting is to keep Graal unit tests as is (located in src/jdk.internal.vm.compiler/share/classes/*.test) and run them similar way they are run in Graal project. To achieve this test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java helper class has been written which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to run specified set of Graal unit tests. The groups of Graal unit tests to run are defined in auto-generated test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests. New make/test/JtregGraalUnit.gmk is used to build Graal unit tests into jdk.vm.compiler.tests.jar and then install it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/. GRAALUNIT_LIB is never defined (in the open). Since this is used in open makefiles, we need an open configure option to set it. ok, I created open/make/autoconf/lib-tests.m4 and did put Graal related staff there. To make things clearer, I would rename LIB_DIR to something like LIB_OUTPUTDIR to signal that this is a location to put files in, rather than read them from. ok, renamed FLATTEN has no effect in the SetupCopyFiles call since all the jar files found by wildcard can only be in one directory anyway. ok, removed BUILDTOOLS_OUTPUTDIR is meant for tools used during the build. Tests classes and jars should be built into $(SUPPORT_OUTPUTDIR)/test/..> Please create a suitable sub directory there for the output from this makefile. ok, introduced COMPILE_OUTPUTDIR := $(SUPPORT_OUTPUTDIR)/test/graalunit to be used to build graal unit test classes. The all and test-image targets are never called so no need to declare them. There are some style issues too. (Please see http://openjdk.java.net/groups/build/doc/code-conventions.html for details.) * 43 logic indent 2 spaces * 52 we like to put the ending )) on a new line with ', \' at the end of the line before * 58 continuation indent 4 spaces * 88, 89 please break long lines * 90 continuation indent 4 spaces * 99 continuation indent 4 spaces * 120 break before )) I think I fixed everything make/Main.gmk adds proper dependencies for build-test-hotspot-jtreg-graal and test-image-hotspot-jtreg-graal. The target build-test-hotspot-jtreg-graal only needs to depend on exploded-image-optimize.
Re: RFR(M): 8205207: Port Graal unit tests under jtreg
Fixed and regenerated webrev at the same location: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html Erik, thanks again for your detailed reviews! -katya On 6/22/18 2:38 PM, Erik Joelsson wrote: Hello Katya, This looks much better, thanks! A few suggestions still: Main.gmk: instead of repeating the assignment in both if and else block: ifeq ($(INCLUDE_GRAAL), true) JVM_TEST_IMAGE_TARGETS += test-image-hotspot-jtreg-graal endif I think it's fine to do that without the ?= because an alternative JVM implementation is unlikely to be using graal anyway. In JtregGraalUnit.gmk: Some minor style nits: 54: align )) with first eval line as you have done with the other eval calls 118: Since 117 is a one line parameter, please move comma to 117 133: Same as for 118 130-132: Please indent 4 spaces for continuation /Erik On 2018-06-22 14:16, Ekaterina Pavlova wrote: Erik, Doug, thank you a lot for your reviews and advises. I fixed everything what Erik has pointed out, please see my answers inlined. As about moving more staff in 'updategraalinopenjdk' can we consider this as next step? I am not quite familiar with 'updategraalinopenjdk' and didn't contribute into Graal ws yet, so I will prefer to do this improvement/refactoring as part of separate JDK issue. New webrev is here: webrev: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8205207 testing: tested by building and running graal unit tests On 6/19/18 2:00 PM, Erik Joelsson wrote: Hello, Please always include build-dev when reviewing build changes. In general this looks pretty good but there are some details that need fixing. (Aside from this particular review, I must state my reservation against all the special treatment graal is enjoying from a source and test layout and build perspective. My understanding here is that someone will have to regularly update the wrapper jtreg tests manually using the script. This in addition to having to implement this very convoluted redirection logic because the tests are in the wrong place. Wouldn't it make a lot more sense to put the complicated logic in the import procedure for graal source so that it would fit nicely into the OpenJDK source/build/test model, instead of adding more and more complicated workarounds in the OpenJDK build and test procedures?) On 2018-06-18 22:26, Ekaterina Pavlova wrote: Hi All, please review porting of Graal unit tests under jtreg. The main idea of this porting is to keep Graal unit tests as is (located in src/jdk.internal.vm.compiler/share/classes/*.test) and run them similar way they are run in Graal project. To achieve this test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java helper class has been written which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to run specified set of Graal unit tests. The groups of Graal unit tests to run are defined in auto-generated test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests. New make/test/JtregGraalUnit.gmk is used to build Graal unit tests into jdk.vm.compiler.tests.jar and then install it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/. GRAALUNIT_LIB is never defined (in the open). Since this is used in open makefiles, we need an open configure option to set it. ok, I created open/make/autoconf/lib-tests.m4 and did put Graal related staff there. To make things clearer, I would rename LIB_DIR to something like LIB_OUTPUTDIR to signal that this is a location to put files in, rather than read them from. ok, renamed FLATTEN has no effect in the SetupCopyFiles call since all the jar files found by wildcard can only be in one directory anyway. ok, removed BUILDTOOLS_OUTPUTDIR is meant for tools used during the build. Tests classes and jars should be built into $(SUPPORT_OUTPUTDIR)/test/..> Please create a suitable sub directory there for the output from this makefile. ok, introduced COMPILE_OUTPUTDIR := $(SUPPORT_OUTPUTDIR)/test/graalunit to be used to build graal unit test classes. The all and test-image targets are never called so no need to declare them. There are some style issues too. (Please see http://openjdk.java.net/groups/build/doc/code-conventions.html for details.) * 43 logic indent 2 spaces * 52 we like to put the ending )) on a new line with ', \' at the end of the line before * 58 continuation indent 4 spaces * 88, 89 please break long lines * 90 continuation indent 4 spaces * 99 continuation indent 4 spaces * 120 break before )) I think I fixed everything make/Main.gmk adds proper dependencies for build-test-hotspot-jtreg-graal and test-image-hotspot-jtreg-graal. The target build-test-hotspot-jtreg-graal only needs to depend on exploded-image-optimize. fixed The new graal specific targets should be guarded by INCLUDE_GRAAL in Main.gmk. Otherwise those targets will silently do nothing i
Re: RFR(M): 8205207: Port Graal unit tests under jtreg
On 6/22/18 2:29 PM, Doug Simon wrote: On 22 Jun 2018, at 23:16, Ekaterina Pavlova wrote: Erik, Doug, thank you a lot for your reviews and advises. I fixed everything what Erik has pointed out, please see my answers inlined. As about moving more staff in 'updategraalinopenjdk' can we consider this as next step? I am not quite familiar with 'updategraalinopenjdk' and didn't contribute into Graal ws yet, so I will prefer to do this improvement/refactoring as part of separate JDK issue. Sorry for not being clearer. I don't expect you to fix/enhance updategraalinopenjdk. I was simply commenting on a possible solution to Joel's reasonabl> request for having Graal sources and test layout more normalized in the JDK code base. ok, good :) -katya
Re: RFR(M): 8205207: Port Graal unit tests under jtreg
Erik, Doug, thank you a lot for your reviews and advises. I fixed everything what Erik has pointed out, please see my answers inlined. As about moving more staff in 'updategraalinopenjdk' can we consider this as next step? I am not quite familiar with 'updategraalinopenjdk' and didn't contribute into Graal ws yet, so I will prefer to do this improvement/refactoring as part of separate JDK issue. New webrev is here: webrev: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8205207 testing: tested by building and running graal unit tests On 6/19/18 2:00 PM, Erik Joelsson wrote: Hello, Please always include build-dev when reviewing build changes. In general this looks pretty good but there are some details that need fixing. (Aside from this particular review, I must state my reservation against all the special treatment graal is enjoying from a source and test layout and build perspective. My understanding here is that someone will have to regularly update the wrapper jtreg tests manually using the script. This in addition to having to implement this very convoluted redirection logic because the tests are in the wrong place. Wouldn't it make a lot more sense to put the complicated logic in the import procedure for graal source so that it would fit nicely into the OpenJDK source/build/test model, instead of adding more and more complicated workarounds in the OpenJDK build and test procedures?) On 2018-06-18 22:26, Ekaterina Pavlova wrote: Hi All, please review porting of Graal unit tests under jtreg. The main idea of this porting is to keep Graal unit tests as is (located in src/jdk.internal.vm.compiler/share/classes/*.test) and run them similar way they are run in Graal project. To achieve this test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java helper class has been written which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to run specified set of Graal unit tests. The groups of Graal unit tests to run are defined in auto-generated test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests. New make/test/JtregGraalUnit.gmk is used to build Graal unit tests into jdk.vm.compiler.tests.jar and then install it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/. GRAALUNIT_LIB is never defined (in the open). Since this is used in open makefiles, we need an open configure option to set it. ok, I created open/make/autoconf/lib-tests.m4 and did put Graal related staff there. To make things clearer, I would rename LIB_DIR to something like LIB_OUTPUTDIR to signal that this is a location to put files in, rather than read them from. ok, renamed FLATTEN has no effect in the SetupCopyFiles call since all the jar files found by wildcard can only be in one directory anyway. ok, removed BUILDTOOLS_OUTPUTDIR is meant for tools used during the build. Tests classes and jars should be built into $(SUPPORT_OUTPUTDIR)/test/..> Please create a suitable sub directory there for the output from this makefile. ok, introduced COMPILE_OUTPUTDIR := $(SUPPORT_OUTPUTDIR)/test/graalunit to be used to build graal unit test classes. The all and test-image targets are never called so no need to declare them. There are some style issues too. (Please see http://openjdk.java.net/groups/build/doc/code-conventions.html for details.) * 43 logic indent 2 spaces * 52 we like to put the ending )) on a new line with ', \' at the end of the line before * 58 continuation indent 4 spaces * 88, 89 please break long lines * 90 continuation indent 4 spaces * 99 continuation indent 4 spaces * 120 break before )) I think I fixed everything make/Main.gmk adds proper dependencies for build-test-hotspot-jtreg-graal and test-image-hotspot-jtreg-graal. The target build-test-hotspot-jtreg-graal only needs to depend on exploded-image-optimize. fixed The new graal specific targets should be guarded by INCLUDE_GRAAL in Main.gmk. Otherwise those targets will silently do nothing if invoked without graal. That means adding them to JVM_TEST_IMAGE_TARGETS needs to be conditional too. fixed test/TestCommon.gmk passes TEST_IMAGE_GRAAL_DIR to jtreg so jtreg knows where to find Graal tests and libs. This needs to be duplicated for make/RunTest.gmk so that these tests work with "make run-test" as well. added thanks, -katya /Erik test/hotspot/jtreg/compiler/graalunit/TestPackages.txt file defines "testName -> testPrefix [requiresStatement]" mapping which is used by test/hotspot/jtreg/compiler/graalunit/generateTests.sh script to generate jtreg tests. test/hotspot/jtreg/compiler/graalunit/com.oracle.mxtool.junit was ported from mx project without any changes. JBS: https://bugs.openjdk.java.net/browse/JDK-8205207 webrev: http://cr.openjdk.java.net/~epavlova//8205207/webrev.00/index.html testing: new tests were executed as part
Re: RFR(S) 8197453 : Add support of extra problem list
Jon, thanks for the review. I have updated the webrev. thanks, -katya On 2/26/18 12:02 PM, Jonathan Gibbons wrote: If these new problem-list files are destined for use by jtreg, I would encourage adding a platform specifier on each line, after the bug number. If you want to mark the test as excluded on all platforms, the convention is to use "generic-all". -- Jon On 2/26/18 11:47 AM, Igor Ignatyev wrote: adding build-dev alias -- Igor On Feb 8, 2018, at 3:08 PM, Ekaterina Pavlova wrote: Hi all, ProblemList.txt files used by makefiles for jtreg testing allow to specify list of tests to be excluded from execution on all or specific platforms. However to test such features like Graal we want to be able to specify list of failed tests which fail in particular JVM mode only. Please review this change which adds support of extra problem list and introduces 2 Graal specific problem list files. - test/hotspot/jtreg/ProblemList-graal.txt - test/jdk/ProblemList-graal.txt JBS: https://bugs.openjdk.java.net/browse/JDK-8197453 webrev: http://cr.openjdk.java.net/~epavlova//8197453/webrev.00/ testing: precheckin, tier1 and tier2 with empty EXTRA_PROBLEM_LISTS. testing in Graal mode with EXTRA_PROBLEM_LISTS=ProblemList-graal.txt thanks, -katya p.s. Igor Ignatyev volunteered to sponsor this change.
