Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Hey Peter, Thank you for looking into this. I planned on coming back to this later, as it wasn't as much of a quick-fix as I'd hoped, but got caught up working on other issues in the mean time. Jorn On 11/07/2020 12:00, Peter Levart wrote: On 7/11/20 9:31 AM, Peter Levart wrote: - compile the two sets of benchmarks separately with separate output directories and create separate benchmarks.jar files for them Here's my attempt at a patch for separate benchmarks.jar files. The minor complication, as I found out, was what to do when running the micro benchmarks via "make test TEST=..." command when there are two possible jar files to choose from. I opted for a separate tag in TEST variable, so preview benchmark(s) are run as follows: make test TEST="micro.preview: ..." http://cr.openjdk.java.net/~plevart/jdk-dev/8248429_jmh_enable_preview/webrev.02/ WDYT? Regards, Peter
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Hello Peter, On 2020-07-11 03:00, Peter Levart wrote: On 7/11/20 9:31 AM, Peter Levart wrote: - compile the two sets of benchmarks separately with separate output directories and create separate benchmarks.jar files for them Here's my attempt at a patch for separate benchmarks.jar files. The minor complication, as I found out, was what to do when running the micro benchmarks via "make test TEST=..." command when there are two possible jar files to choose from. I opted for a separate tag in TEST variable, so preview benchmark(s) are run as follows: make test TEST="micro.preview: ..." http://cr.openjdk.java.net/~plevart/jdk-dev/8248429_jmh_enable_preview/webrev.02/ This generally looks pretty good to me. I will defer to Claes on what build steps are actually needed for the new jar file. I agree with Claes that having a list of files in the makefile is probably easier to update than moving files around. I would recommend something like this: MICROBENCHMARK_PREVIEW_FILES := \ org/openjdk/bench/java/io/RecordDeserialization.java \ # This variable can then be fed into EXCLUDE_FILES and INCLUDE_FILES respectively. The formatting is our recommended way of building lists of things in the makefiles where lines can easily be added or removed without affecting any adjacent lines to minimize merge conflicts in the future. Otherwise, I would only ask that you try to shorten some lines a bit. We aren't strictly enforcing 80 chars, but try to stay close when possible for easier side by side reviews and 3-way merges in the future. Specifically RunTests.gmk:682 (maybe an intermediate value?) and BuildMicrobenchmark.gmk:197. /Erik WDYT? Regards, Peter
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Hi, I think the idea of generating two distinct jars is workable, but I really don't like the prospect of routinely having to move micros around as the features they test promote from preview. Would a manually managed list of which tests use --enable-preview work for you? Not ideal either, but that would be less intrusive when taking the feature out of preview along with removal of the flag from the micro settings. Also, I believe the INDIFY step to be an unfortunate piece of technical debt which we don't need to carry over to a preview build step. Thanks! /Claes On 2020-07-11 12:00, Peter Levart wrote: On 7/11/20 9:31 AM, Peter Levart wrote: - compile the two sets of benchmarks separately with separate output directories and create separate benchmarks.jar files for them Here's my attempt at a patch for separate benchmarks.jar files. The minor complication, as I found out, was what to do when running the micro benchmarks via "make test TEST=..." command when there are two possible jar files to choose from. I opted for a separate tag in TEST variable, so preview benchmark(s) are run as follows: make test TEST="micro.preview: ..." http://cr.openjdk.java.net/~plevart/jdk-dev/8248429_jmh_enable_preview/webrev.02/ WDYT? Regards, Peter
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 7/11/20 9:31 AM, Peter Levart wrote: - compile the two sets of benchmarks separately with separate output directories and create separate benchmarks.jar files for them Here's my attempt at a patch for separate benchmarks.jar files. The minor complication, as I found out, was what to do when running the micro benchmarks via "make test TEST=..." command when there are two possible jar files to choose from. I opted for a separate tag in TEST variable, so preview benchmark(s) are run as follows: make test TEST="micro.preview: ..." http://cr.openjdk.java.net/~plevart/jdk-dev/8248429_jmh_enable_preview/webrev.02/ WDYT? Regards, Peter
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 7/7/20 7:51 PM, Roger Riggs wrote: Hi, I have found it very useful to be able to run benchmarks against multiple versions of the JDK. Build the benchmark jar once and to compare results. If all of the classes are built with --enable-preview, none of them will run against older JDKs. So an approach that only compiles those files that need preview would be more useful. ... Or perhaps, separate out preview based benchmarks into a separate jar. $.02, Roger Perhaps the last option is the way to go. Why? Preview features are specific to a major OpenJDK release so while a particular preview feature may be present as a preview feature in two consecutive releases, you can not run a class with the preview feature compiled for OpenJDK N on an OpenJDK N+1 or vice versa. So a single benchmarks.jar file makes sense only for benchmarks that don't use preview features and are compiled with (or for) lowest release possible... The benchmarks using preview feature OTOH will need to be re-classified once the preview feature graduates and becomes a mainline feature. So once it is classified differently in OpenJDK N+1 as it was in OpenJDK N, it can not be built with such classification on (or for) the OpenJDK N release any more because, among other things, names of packages/modules may change. So these two sets of benchmarks (using or not using preview features) have different constraints and is therefore reasonable for them to be built separately and packed into separate benchmarks.jar files. Packing to separate .jar files might be a pragmatic solution for a problem that Jorn Vernee discovered: the JMH compiler plugin generates two files in the output directory which are included in the benchmarks.jar: /META-INF/BenchmarkList /META-INF/CompilerHints While the 1st one is "updated" incrementally if it already exists, its modification is not protected by any kind of locking mechanism and so concurrent compilation by two or more instances of javac may produce garbled result. The 2nd one seems to be overwritten entirely by content of a single compile session, so its final form does not represent compiler hints for all aggregated benchmarks and may therefore produce skewed results for some benchmarks. I see two solutions for that problem: - fix JMH to correctly handle concurrent incremental updates to both above files and; or - compile the two sets of benchmarks separately with separate output directories and create separate benchmarks.jar files for them I think the 2nd option is a simpler, pragmatic solution. Peter On 7/7/20 10:26 AM, Peter Levart wrote: I suggest adding --enable-preview to JMH_JVM_ARGS in general now (it doesn't hurt even if classes are not compiled with --enable-preview) and then take time to devise an effective strategy for selectively compiling micro benchmarks with or without --enable-preview. At least so the benchmarks would work out-of-the-box when run via make test. WDYT? Regards, Peter On 6/30/20 10:15 PM, Claes Redestad wrote: On 2020-06-30 22:12, Magnus Ihse Bursie wrote: Second to that a solution in the build would be preferable - if we can come up with something that has infinitesimal impact to build times. Are we talking about many files? Could you consider listing those files explicitly in the makefile? That would make it cheap to filter them out from the normal compilation, and instead do a secondary compilation with them. Right now there's one micro using --enable-preview, so that'd be a very short list. /Claes
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Hi, I have found it very useful to be able to run benchmarks against multiple versions of the JDK. Build the benchmark jar once and to compare results. If all of the classes are built with --enable-preview, none of them will run against older JDKs. So an approach that only compiles those files that need preview would be more useful. Or perhaps, separate out preview based benchmarks into a separate jar. $.02, Roger On 7/7/20 10:26 AM, Peter Levart wrote: I suggest adding --enable-preview to JMH_JVM_ARGS in general now (it doesn't hurt even if classes are not compiled with --enable-preview) and then take time to devise an effective strategy for selectively compiling micro benchmarks with or without --enable-preview. At least so the benchmarks would work out-of-the-box when run via make test. WDYT? Regards, Peter On 6/30/20 10:15 PM, Claes Redestad wrote: On 2020-06-30 22:12, Magnus Ihse Bursie wrote: Second to that a solution in the build would be preferable - if we can come up with something that has infinitesimal impact to build times. Are we talking about many files? Could you consider listing those files explicitly in the makefile? That would make it cheap to filter them out from the normal compilation, and instead do a secondary compilation with them. Right now there's one micro using --enable-preview, so that'd be a very short list. /Claes
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 30/06/2020 22:51, Erik Joelsson wrote: On 2020-06-30 13:15, Claes Redestad wrote: On 2020-06-30 22:12, Magnus Ihse Bursie wrote: Second to that a solution in the build would be preferable - if we can come up with something that has infinitesimal impact to build times. Are we talking about many files? Could you consider listing those files explicitly in the makefile? That would make it cheap to filter them out from the normal compilation, and instead do a secondary compilation with them. Right now there's one micro using --enable-preview, so that'd be a very short list. Having to update a list manually is no fun, but at least we can make the build fail if you forget. If you only disable the warning "preview" for the special compilation set, the build would fail if another microbenchmark was added that needed it. A little bit of an update on this. I've implemented the manual list of preview benchmarks [1], but there's a problem with the current patch. Both the BUILD_JDK_MICROBENCHMARK and the BUILD_JDK_MICROBENCHMARK_PREVIEW have the same output folder, and this is good, since we want only 1 benchmarks.jar, we want the compilation result to be combined. But the JMH annotation processor is also generating some metadata files in META-INF, namely BenchmarkList and CompilerHints. The problem is that both compilation tasks seem to be trying to write to these files at the same time, leading to e.g. the RecordsDeserialization benchmark not appearing in the final BenchmarkList, or seemingly resulting in a mangled file, crashing the JMH parser. I've tried to resolve this race be adding a dependency on BUILD_JDK_MICROBENCHMARK to BUILD_JDK_MICROBENCHMARK_PREVIEW (see the patch), but this doesn't resolve the problem. I'm still investigating this. Jorn [1] : http://cr.openjdk.java.net/~jvernee/8248429/webrev.02/ /Erik /Claes
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 7/7/20 4:26 PM, Peter Levart wrote: I suggest adding --enable-preview to JMH_JVM_ARGS in general now (it doesn't hurt even if classes are not compiled with --enable-preview) and then take time to devise an effective strategy for selectively compiling micro benchmarks with or without --enable-preview. At least so the benchmarks would work out-of-the-box when run via make test. WDYT? Or maybe this variant is acceptable? http://cr.openjdk.java.net/~plevart/jdk-dev/8248429_jmh_enable_preview/webrev.01/ I took Jorn Vernee's patch and modified it a bit so that it does not need to find and grep the files but I rather specify relative source directories for EXCLUDES and INCLUDES parameters to the two separate SetupJavaCompilation tasks respectively. Benchmarks that need preview features (currently just one) then need to be in a package with special prefix org.openjdk.bench.preview so they are separately compiled with --enable-preview option. Other files are compiled without it. Peter Regards, Peter On 6/30/20 10:15 PM, Claes Redestad wrote: On 2020-06-30 22:12, Magnus Ihse Bursie wrote: Second to that a solution in the build would be preferable - if we can come up with something that has infinitesimal impact to build times. Are we talking about many files? Could you consider listing those files explicitly in the makefile? That would make it cheap to filter them out from the normal compilation, and instead do a secondary compilation with them. Right now there's one micro using --enable-preview, so that'd be a very short list. /Claes
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
I suggest adding --enable-preview to JMH_JVM_ARGS in general now (it doesn't hurt even if classes are not compiled with --enable-preview) and then take time to devise an effective strategy for selectively compiling micro benchmarks with or without --enable-preview. At least so the benchmarks would work out-of-the-box when run via make test. WDYT? Regards, Peter On 6/30/20 10:15 PM, Claes Redestad wrote: On 2020-06-30 22:12, Magnus Ihse Bursie wrote: Second to that a solution in the build would be preferable - if we can come up with something that has infinitesimal impact to build times. Are we talking about many files? Could you consider listing those files explicitly in the makefile? That would make it cheap to filter them out from the normal compilation, and instead do a secondary compilation with them. Right now there's one micro using --enable-preview, so that'd be a very short list. /Claes
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 2020-06-30 13:15, Claes Redestad wrote: On 2020-06-30 22:12, Magnus Ihse Bursie wrote: Second to that a solution in the build would be preferable - if we can come up with something that has infinitesimal impact to build times. Are we talking about many files? Could you consider listing those files explicitly in the makefile? That would make it cheap to filter them out from the normal compilation, and instead do a secondary compilation with them. Right now there's one micro using --enable-preview, so that'd be a very short list. Having to update a list manually is no fun, but at least we can make the build fail if you forget. If you only disable the warning "preview" for the special compilation set, the build would fail if another microbenchmark was added that needed it. /Erik /Claes
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 2020-06-30 22:12, Magnus Ihse Bursie wrote: Second to that a solution in the build would be preferable - if we can come up with something that has infinitesimal impact to build times. Are we talking about many files? Could you consider listing those files explicitly in the makefile? That would make it cheap to filter them out from the normal compilation, and instead do a secondary compilation with them. Right now there's one micro using --enable-preview, so that'd be a very short list. /Claes
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 2020-06-30 19:32, Claes Redestad wrote: On 2020-06-30 18:40, Magnus Ihse Bursie wrote: An alternative workaround would be to add @Fork(jvmArgsAppend = "--enable-preview") to all micros, whether they use preview features or not. Perhaps that wouldn't be so bad, actually. That sounds like a reasonable compromise, yes. Well, it moves the responsibility to micro authors, while violating the principle of least surprise in some ways. I think my preference would be some means to ask javac to not tag .class files with the 65535 minor version unless there's some actual preview feature usage (which I think is what the --enable-preview flag should do by default, but I guess someone has insisted on the current behavior to get more of a fail fast behavior). Second to that a solution in the build would be preferable - if we can come up with something that has infinitesimal impact to build times. Are we talking about many files? Could you consider listing those files explicitly in the makefile? That would make it cheap to filter them out from the normal compilation, and instead do a secondary compilation with them. /Magnus Adding --enable-preview to every micro would be a reasonable workaround, for now, but I view it as a temporary hack. /Claes
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 2020-06-30 18:40, Magnus Ihse Bursie wrote: An alternative workaround would be to add @Fork(jvmArgsAppend = "--enable-preview") to all micros, whether they use preview features or not. Perhaps that wouldn't be so bad, actually. That sounds like a reasonable compromise, yes. Well, it moves the responsibility to micro authors, while violating the principle of least surprise in some ways. I think my preference would be some means to ask javac to not tag .class files with the 65535 minor version unless there's some actual preview feature usage (which I think is what the --enable-preview flag should do by default, but I guess someone has insisted on the current behavior to get more of a fail fast behavior). Second to that a solution in the build would be preferable - if we can come up with something that has infinitesimal impact to build times. Adding --enable-preview to every micro would be a reasonable workaround, for now, but I view it as a temporary hack. /Claes
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 2020-06-30 18:19, Claes Redestad wrote: On 2020-06-30 17:16, Magnus Ihse Bursie wrote: On 2020-06-30 16:48, Erik Joelsson wrote: On 2020-06-30 07:15, Magnus Ihse Bursie wrote: On 2020-06-30 15:13, Claes Redestad wrote: Hi Jorn, On 2020-06-30 14:52, Jorn Vernee wrote: Hi Claes, I see what you mean. I've created a patch that instead greps through all the benchmark source files, and finds files with `--enable-preview` in them. Then, only those files are compiled with --enable-preview, by using a separate call to SetupJavaCompilation. This relies on the fact that the benchmarks that use preview features also use `@Fork(... jvmAppendArgs= "--enable-preview")`, but, maybe a different marker can be used to mark benchmarks that need to be compiled with --enable-preview as well. Alternatively, we could use 2 separate directory structures to house preview and non-preview benchmarks. WDYT? Webrev: http://cr.openjdk.java.net/~jvernee/8248429/webrev.00/ this looks great to me! Not to rain on your parade, but I'm very skeptical of this. :-( Even just finding files is slow as it is on Windows (and we try hard to avoid that as much as possible during the build), greping through all files is even slower. I'm also concerned about build performance, and in this context incremental build performance. We try very hard to avoid $(shell) calls in the makefiles, especially if these are used to generate rules as those calls must then be made every time make is invoked to figure out what files need to be rebuilt. Sometimes it's unavoidable, and we do what we can to just minimize the damage. We need to be clear on this being a worthwhile strategy before proceeding. Would it be possible to implement this filtering as a javac plugin instead? That would reduce the need to process files just to figure out if compilation is needed. We could also emulate this partly in make by moving the SetupNativeCompilation calls into a sub make call and only call those if any java file is newer than the compilation target. This would result in a very convoluted makefile, but might be worth it. All this sounds very convoluted. I think the original patch was good enough, and I'd like to hear Claes argument again why all this complicated setup is needed. If it was just "well the other tests don't really need it", I think we can reply with "well, it doesn't hurt either, and it's too costly to apply it individually". as it turns out, javac --enable-preview tags all compiled classes as being preview enabled (classfile minor version 65535), so you now need to run java with --enable-preview to run any microbenchmark. This is a regression for existing microbenchmarks (one which I'm responsible for). Ok. Jorn's patch here fixes that regression: no --enable-preview will be required to neither make test nor java -jar ../benchmarks.jar runs, thanks to how JMH runs the --enable-preview compiled benchmark code only in forks. An alternative workaround would be to add @Fork(jvmArgsAppend = "--enable-preview") to all micros, whether they use preview features or not. Perhaps that wouldn't be so bad, actually. That sounds like a reasonable compromise, yes. /Magnus /Claes /Magnus Now to the patch. One thing that can be done in this patch to minimize damage is to use the RelativePath macro from Utils.gmk instead of calling realpath in the shell. I would also avoid building a full list of all java files and sending them to grep on a single command line. Probably better to either find into xargs or just let grep work recursively. Also please keep line length down so that future side by side comparisons as well as 3-way merges are still possible. We aren't strict on 80, but try to aim close to it. On a related note, how does this --enable-preview work when running the microbenchmarks as a jtreg test? /Erik Remind me again what problem this was supposed to solve that just adding --enable-preview to the compilation didn't solve..? /Magnus Testing: deleting build//support/test/micro and build//images/test/micro and confirming that compiling and running benchmarks with and without preview features works as expected. I don't think there's any automated tests for benchmarks right? Not really, no, but a tier1 build and test would at least build the micros and detect any issues with your shell calls on our range of platforms. While we support running the per-build micro artifacts in our internal benchmarking system on an adhoc basis, our promotion testing (where this would have been discovered) is still a bit semi-automatic in regards to when we roll over to a new benchmarks.jar. /Claes Jorn On 27/06/2020 01:38, Claes Redestad wrote: Patch looks fine (although you might want to update the comment). It's more concerning that I didn't catch this (seems all tests of mine were with --enable-preview), and we'll still inconvenience users who need to run the jar file directly.
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 2020-06-30 17:16, Magnus Ihse Bursie wrote: On 2020-06-30 16:48, Erik Joelsson wrote: On 2020-06-30 07:15, Magnus Ihse Bursie wrote: On 2020-06-30 15:13, Claes Redestad wrote: Hi Jorn, On 2020-06-30 14:52, Jorn Vernee wrote: Hi Claes, I see what you mean. I've created a patch that instead greps through all the benchmark source files, and finds files with `--enable-preview` in them. Then, only those files are compiled with --enable-preview, by using a separate call to SetupJavaCompilation. This relies on the fact that the benchmarks that use preview features also use `@Fork(... jvmAppendArgs= "--enable-preview")`, but, maybe a different marker can be used to mark benchmarks that need to be compiled with --enable-preview as well. Alternatively, we could use 2 separate directory structures to house preview and non-preview benchmarks. WDYT? Webrev: http://cr.openjdk.java.net/~jvernee/8248429/webrev.00/ this looks great to me! Not to rain on your parade, but I'm very skeptical of this. :-( Even just finding files is slow as it is on Windows (and we try hard to avoid that as much as possible during the build), greping through all files is even slower. I'm also concerned about build performance, and in this context incremental build performance. We try very hard to avoid $(shell) calls in the makefiles, especially if these are used to generate rules as those calls must then be made every time make is invoked to figure out what files need to be rebuilt. Sometimes it's unavoidable, and we do what we can to just minimize the damage. We need to be clear on this being a worthwhile strategy before proceeding. Would it be possible to implement this filtering as a javac plugin instead? That would reduce the need to process files just to figure out if compilation is needed. We could also emulate this partly in make by moving the SetupNativeCompilation calls into a sub make call and only call those if any java file is newer than the compilation target. This would result in a very convoluted makefile, but might be worth it. All this sounds very convoluted. I think the original patch was good enough, and I'd like to hear Claes argument again why all this complicated setup is needed. If it was just "well the other tests don't really need it", I think we can reply with "well, it doesn't hurt either, and it's too costly to apply it individually". as it turns out, javac --enable-preview tags all compiled classes as being preview enabled (classfile minor version 65535), so you now need to run java with --enable-preview to run any microbenchmark. This is a regression for existing microbenchmarks (one which I'm responsible for). Jorn's patch here fixes that regression: no --enable-preview will be required to neither make test nor java -jar ../benchmarks.jar runs, thanks to how JMH runs the --enable-preview compiled benchmark code only in forks. An alternative workaround would be to add @Fork(jvmArgsAppend = "--enable-preview") to all micros, whether they use preview features or not. Perhaps that wouldn't be so bad, actually. /Claes /Magnus Now to the patch. One thing that can be done in this patch to minimize damage is to use the RelativePath macro from Utils.gmk instead of calling realpath in the shell. I would also avoid building a full list of all java files and sending them to grep on a single command line. Probably better to either find into xargs or just let grep work recursively. Also please keep line length down so that future side by side comparisons as well as 3-way merges are still possible. We aren't strict on 80, but try to aim close to it. On a related note, how does this --enable-preview work when running the microbenchmarks as a jtreg test? /Erik Remind me again what problem this was supposed to solve that just adding --enable-preview to the compilation didn't solve..? /Magnus Testing: deleting build//support/test/micro and build//images/test/micro and confirming that compiling and running benchmarks with and without preview features works as expected. I don't think there's any automated tests for benchmarks right? Not really, no, but a tier1 build and test would at least build the micros and detect any issues with your shell calls on our range of platforms. While we support running the per-build micro artifacts in our internal benchmarking system on an adhoc basis, our promotion testing (where this would have been discovered) is still a bit semi-automatic in regards to when we roll over to a new benchmarks.jar. /Claes Jorn On 27/06/2020 01:38, Claes Redestad wrote: Patch looks fine (although you might want to update the comment). It's more concerning that I didn't catch this (seems all tests of mine were with --enable-preview), and we'll still inconvenience users who need to run the jar file directly. So it seems we should consider fixing so that only those benchmarks that actually need --enable-preview are
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 2020-06-30 16:48, Erik Joelsson wrote: On 2020-06-30 07:15, Magnus Ihse Bursie wrote: On 2020-06-30 15:13, Claes Redestad wrote: Hi Jorn, On 2020-06-30 14:52, Jorn Vernee wrote: Hi Claes, I see what you mean. I've created a patch that instead greps through all the benchmark source files, and finds files with `--enable-preview` in them. Then, only those files are compiled with --enable-preview, by using a separate call to SetupJavaCompilation. This relies on the fact that the benchmarks that use preview features also use `@Fork(... jvmAppendArgs= "--enable-preview")`, but, maybe a different marker can be used to mark benchmarks that need to be compiled with --enable-preview as well. Alternatively, we could use 2 separate directory structures to house preview and non-preview benchmarks. WDYT? Webrev: http://cr.openjdk.java.net/~jvernee/8248429/webrev.00/ this looks great to me! Not to rain on your parade, but I'm very skeptical of this. :-( Even just finding files is slow as it is on Windows (and we try hard to avoid that as much as possible during the build), greping through all files is even slower. I'm also concerned about build performance, and in this context incremental build performance. We try very hard to avoid $(shell) calls in the makefiles, especially if these are used to generate rules as those calls must then be made every time make is invoked to figure out what files need to be rebuilt. Sometimes it's unavoidable, and we do what we can to just minimize the damage. We need to be clear on this being a worthwhile strategy before proceeding. Would it be possible to implement this filtering as a javac plugin instead? That would reduce the need to process files just to figure out if compilation is needed. We could also emulate this partly in make by moving the SetupNativeCompilation calls into a sub make call and only call those if any java file is newer than the compilation target. This would result in a very convoluted makefile, but might be worth it. All this sounds very convoluted. I think the original patch was good enough, and I'd like to hear Claes argument again why all this complicated setup is needed. If it was just "well the other tests don't really need it", I think we can reply with "well, it doesn't hurt either, and it's too costly to apply it individually". /Magnus Now to the patch. One thing that can be done in this patch to minimize damage is to use the RelativePath macro from Utils.gmk instead of calling realpath in the shell. I would also avoid building a full list of all java files and sending them to grep on a single command line. Probably better to either find into xargs or just let grep work recursively. Also please keep line length down so that future side by side comparisons as well as 3-way merges are still possible. We aren't strict on 80, but try to aim close to it. On a related note, how does this --enable-preview work when running the microbenchmarks as a jtreg test? /Erik Remind me again what problem this was supposed to solve that just adding --enable-preview to the compilation didn't solve..? /Magnus Testing: deleting build//support/test/micro and build//images/test/micro and confirming that compiling and running benchmarks with and without preview features works as expected. I don't think there's any automated tests for benchmarks right? Not really, no, but a tier1 build and test would at least build the micros and detect any issues with your shell calls on our range of platforms. While we support running the per-build micro artifacts in our internal benchmarking system on an adhoc basis, our promotion testing (where this would have been discovered) is still a bit semi-automatic in regards to when we roll over to a new benchmarks.jar. /Claes Jorn On 27/06/2020 01:38, Claes Redestad wrote: Patch looks fine (although you might want to update the comment). It's more concerning that I didn't catch this (seems all tests of mine were with --enable-preview), and we'll still inconvenience users who need to run the jar file directly. So it seems we should consider fixing so that only those benchmarks that actually need --enable-preview are built with that flag. /Claes On 2020-06-27 00:21, Jorn Vernee wrote: Forgot to attach the JBS link: https://bugs.openjdk.java.net/browse/JDK-8248429 Jorn On 27/06/2020 00:14, Jorn Vernee wrote: Hi, https://bugs.openjdk.java.net/browse/JDK-8248135 added --enable-preview to the javac options when building micro benchmarks. We should also add it to the set of default VM arguments passed to the microbenchmark jar so it doesn't need to be passed manually. If --enable-preview is not passed, the microbenchmarks can not be run (even the ones that don't use preview features). Since the class files have a modified minor version due to building with --enable-preview, the VM must also be started with --enable-preview in order to be able
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 2020-06-30 07:15, Magnus Ihse Bursie wrote: On 2020-06-30 15:13, Claes Redestad wrote: Hi Jorn, On 2020-06-30 14:52, Jorn Vernee wrote: Hi Claes, I see what you mean. I've created a patch that instead greps through all the benchmark source files, and finds files with `--enable-preview` in them. Then, only those files are compiled with --enable-preview, by using a separate call to SetupJavaCompilation. This relies on the fact that the benchmarks that use preview features also use `@Fork(... jvmAppendArgs= "--enable-preview")`, but, maybe a different marker can be used to mark benchmarks that need to be compiled with --enable-preview as well. Alternatively, we could use 2 separate directory structures to house preview and non-preview benchmarks. WDYT? Webrev: http://cr.openjdk.java.net/~jvernee/8248429/webrev.00/ this looks great to me! Not to rain on your parade, but I'm very skeptical of this. :-( Even just finding files is slow as it is on Windows (and we try hard to avoid that as much as possible during the build), greping through all files is even slower. I'm also concerned about build performance, and in this context incremental build performance. We try very hard to avoid $(shell) calls in the makefiles, especially if these are used to generate rules as those calls must then be made every time make is invoked to figure out what files need to be rebuilt. Sometimes it's unavoidable, and we do what we can to just minimize the damage. We need to be clear on this being a worthwhile strategy before proceeding. Would it be possible to implement this filtering as a javac plugin instead? That would reduce the need to process files just to figure out if compilation is needed. We could also emulate this partly in make by moving the SetupNativeCompilation calls into a sub make call and only call those if any java file is newer than the compilation target. This would result in a very convoluted makefile, but might be worth it. Now to the patch. One thing that can be done in this patch to minimize damage is to use the RelativePath macro from Utils.gmk instead of calling realpath in the shell. I would also avoid building a full list of all java files and sending them to grep on a single command line. Probably better to either find into xargs or just let grep work recursively. Also please keep line length down so that future side by side comparisons as well as 3-way merges are still possible. We aren't strict on 80, but try to aim close to it. On a related note, how does this --enable-preview work when running the microbenchmarks as a jtreg test? /Erik Remind me again what problem this was supposed to solve that just adding --enable-preview to the compilation didn't solve..? /Magnus Testing: deleting build//support/test/micro and build//images/test/micro and confirming that compiling and running benchmarks with and without preview features works as expected. I don't think there's any automated tests for benchmarks right? Not really, no, but a tier1 build and test would at least build the micros and detect any issues with your shell calls on our range of platforms. While we support running the per-build micro artifacts in our internal benchmarking system on an adhoc basis, our promotion testing (where this would have been discovered) is still a bit semi-automatic in regards to when we roll over to a new benchmarks.jar. /Claes Jorn On 27/06/2020 01:38, Claes Redestad wrote: Patch looks fine (although you might want to update the comment). It's more concerning that I didn't catch this (seems all tests of mine were with --enable-preview), and we'll still inconvenience users who need to run the jar file directly. So it seems we should consider fixing so that only those benchmarks that actually need --enable-preview are built with that flag. /Claes On 2020-06-27 00:21, Jorn Vernee wrote: Forgot to attach the JBS link: https://bugs.openjdk.java.net/browse/JDK-8248429 Jorn On 27/06/2020 00:14, Jorn Vernee wrote: Hi, https://bugs.openjdk.java.net/browse/JDK-8248135 added --enable-preview to the javac options when building micro benchmarks. We should also add it to the set of default VM arguments passed to the microbenchmark jar so it doesn't need to be passed manually. If --enable-preview is not passed, the microbenchmarks can not be run (even the ones that don't use preview features). Since the class files have a modified minor version due to building with --enable-preview, the VM must also be started with --enable-preview in order to be able to load the classes. In the absence of --enable-preview, for instance the following error will occur: java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/jdk/incubator/foreign/generated/CallOverhead_panama_args10_jmhTest (class file version 60.65535). Try running with '--enable-preview' at
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 2020-06-30 15:13, Claes Redestad wrote: Hi Jorn, On 2020-06-30 14:52, Jorn Vernee wrote: Hi Claes, I see what you mean. I've created a patch that instead greps through all the benchmark source files, and finds files with `--enable-preview` in them. Then, only those files are compiled with --enable-preview, by using a separate call to SetupJavaCompilation. This relies on the fact that the benchmarks that use preview features also use `@Fork(... jvmAppendArgs= "--enable-preview")`, but, maybe a different marker can be used to mark benchmarks that need to be compiled with --enable-preview as well. Alternatively, we could use 2 separate directory structures to house preview and non-preview benchmarks. WDYT? Webrev: http://cr.openjdk.java.net/~jvernee/8248429/webrev.00/ this looks great to me! Not to rain on your parade, but I'm very skeptical of this. :-( Even just finding files is slow as it is on Windows (and we try hard to avoid that as much as possible during the build), greping through all files is even slower. Remind me again what problem this was supposed to solve that just adding --enable-preview to the compilation didn't solve..? /Magnus Testing: deleting build//support/test/micro and build//images/test/micro and confirming that compiling and running benchmarks with and without preview features works as expected. I don't think there's any automated tests for benchmarks right? Not really, no, but a tier1 build and test would at least build the micros and detect any issues with your shell calls on our range of platforms. While we support running the per-build micro artifacts in our internal benchmarking system on an adhoc basis, our promotion testing (where this would have been discovered) is still a bit semi-automatic in regards to when we roll over to a new benchmarks.jar. /Claes Jorn On 27/06/2020 01:38, Claes Redestad wrote: Patch looks fine (although you might want to update the comment). It's more concerning that I didn't catch this (seems all tests of mine were with --enable-preview), and we'll still inconvenience users who need to run the jar file directly. So it seems we should consider fixing so that only those benchmarks that actually need --enable-preview are built with that flag. /Claes On 2020-06-27 00:21, Jorn Vernee wrote: Forgot to attach the JBS link: https://bugs.openjdk.java.net/browse/JDK-8248429 Jorn On 27/06/2020 00:14, Jorn Vernee wrote: Hi, https://bugs.openjdk.java.net/browse/JDK-8248135 added --enable-preview to the javac options when building micro benchmarks. We should also add it to the set of default VM arguments passed to the microbenchmark jar so it doesn't need to be passed manually. If --enable-preview is not passed, the microbenchmarks can not be run (even the ones that don't use preview features). Since the class files have a modified minor version due to building with --enable-preview, the VM must also be started with --enable-preview in order to be able to load the classes. In the absence of --enable-preview, for instance the following error will occur: java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/jdk/incubator/foreign/generated/CallOverhead_panama_args10_jmhTest (class file version 60.65535). Try running with '--enable-preview' at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1016) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:151) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:825) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:723) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:646) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:604) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:168) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:377) at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:233) at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:139) at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:76) at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) Please review the patch attached inline at [1]. Testing: running a microbenchmark without passing
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Hi Jorn, On 2020-06-30 14:52, Jorn Vernee wrote: Hi Claes, I see what you mean. I've created a patch that instead greps through all the benchmark source files, and finds files with `--enable-preview` in them. Then, only those files are compiled with --enable-preview, by using a separate call to SetupJavaCompilation. This relies on the fact that the benchmarks that use preview features also use `@Fork(... jvmAppendArgs= "--enable-preview")`, but, maybe a different marker can be used to mark benchmarks that need to be compiled with --enable-preview as well. Alternatively, we could use 2 separate directory structures to house preview and non-preview benchmarks. WDYT? Webrev: http://cr.openjdk.java.net/~jvernee/8248429/webrev.00/ this looks great to me! Testing: deleting build//support/test/micro and build//images/test/micro and confirming that compiling and running benchmarks with and without preview features works as expected. I don't think there's any automated tests for benchmarks right? Not really, no, but a tier1 build and test would at least build the micros and detect any issues with your shell calls on our range of platforms. While we support running the per-build micro artifacts in our internal benchmarking system on an adhoc basis, our promotion testing (where this would have been discovered) is still a bit semi-automatic in regards to when we roll over to a new benchmarks.jar. /Claes Jorn On 27/06/2020 01:38, Claes Redestad wrote: Patch looks fine (although you might want to update the comment). It's more concerning that I didn't catch this (seems all tests of mine were with --enable-preview), and we'll still inconvenience users who need to run the jar file directly. So it seems we should consider fixing so that only those benchmarks that actually need --enable-preview are built with that flag. /Claes On 2020-06-27 00:21, Jorn Vernee wrote: Forgot to attach the JBS link: https://bugs.openjdk.java.net/browse/JDK-8248429 Jorn On 27/06/2020 00:14, Jorn Vernee wrote: Hi, https://bugs.openjdk.java.net/browse/JDK-8248135 added --enable-preview to the javac options when building micro benchmarks. We should also add it to the set of default VM arguments passed to the microbenchmark jar so it doesn't need to be passed manually. If --enable-preview is not passed, the microbenchmarks can not be run (even the ones that don't use preview features). Since the class files have a modified minor version due to building with --enable-preview, the VM must also be started with --enable-preview in order to be able to load the classes. In the absence of --enable-preview, for instance the following error will occur: java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/jdk/incubator/foreign/generated/CallOverhead_panama_args10_jmhTest (class file version 60.65535). Try running with '--enable-preview' at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1016) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:151) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:825) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:723) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:646) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:604) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:168) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:377) at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:233) at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:139) at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:76) at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) Please review the patch attached inline at [1]. Testing: running a microbenchmark without passing '--enable-preview' manually and confirming that it doesn't fail to load the classes. Thanks, Jorn [1] : diff --git a/make/RunTests.gmk b/make/RunTests.gmk index 721bb827639..59911d89e9f 100644 --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -691,7 +691,7 @@ define SetupRunMicroTestBody endif # Set library path for native dependencies - $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native +
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Hi Claes, I see what you mean. I've created a patch that instead greps through all the benchmark source files, and finds files with `--enable-preview` in them. Then, only those files are compiled with --enable-preview, by using a separate call to SetupJavaCompilation. This relies on the fact that the benchmarks that use preview features also use `@Fork(... jvmAppendArgs= "--enable-preview")`, but, maybe a different marker can be used to mark benchmarks that need to be compiled with --enable-preview as well. Alternatively, we could use 2 separate directory structures to house preview and non-preview benchmarks. WDYT? Webrev: http://cr.openjdk.java.net/~jvernee/8248429/webrev.00/ Testing: deleting build//support/test/micro and build//images/test/micro and confirming that compiling and running benchmarks with and without preview features works as expected. I don't think there's any automated tests for benchmarks right? Jorn On 27/06/2020 01:38, Claes Redestad wrote: Patch looks fine (although you might want to update the comment). It's more concerning that I didn't catch this (seems all tests of mine were with --enable-preview), and we'll still inconvenience users who need to run the jar file directly. So it seems we should consider fixing so that only those benchmarks that actually need --enable-preview are built with that flag. /Claes On 2020-06-27 00:21, Jorn Vernee wrote: Forgot to attach the JBS link: https://bugs.openjdk.java.net/browse/JDK-8248429 Jorn On 27/06/2020 00:14, Jorn Vernee wrote: Hi, https://bugs.openjdk.java.net/browse/JDK-8248135 added --enable-preview to the javac options when building micro benchmarks. We should also add it to the set of default VM arguments passed to the microbenchmark jar so it doesn't need to be passed manually. If --enable-preview is not passed, the microbenchmarks can not be run (even the ones that don't use preview features). Since the class files have a modified minor version due to building with --enable-preview, the VM must also be started with --enable-preview in order to be able to load the classes. In the absence of --enable-preview, for instance the following error will occur: java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/jdk/incubator/foreign/generated/CallOverhead_panama_args10_jmhTest (class file version 60.65535). Try running with '--enable-preview' at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1016) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:151) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:825) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:723) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:646) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:604) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:168) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:377) at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:233) at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:139) at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:76) at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) Please review the patch attached inline at [1]. Testing: running a microbenchmark without passing '--enable-preview' manually and confirming that it doesn't fail to load the classes. Thanks, Jorn [1] : diff --git a/make/RunTests.gmk b/make/RunTests.gmk index 721bb827639..59911d89e9f 100644 --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -691,7 +691,7 @@ define SetupRunMicroTestBody endif # Set library path for native dependencies - $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native + $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native --enable-preview ifneq ($$(MICRO_VM_OPTIONS)$$(MICRO_JAVA_OPTIONS), ) $1_JMH_JVM_ARGS += $$(MICRO_VM_OPTIONS) $$(MICRO_JAVA_OPTIONS)
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Looks good. /Erik On 2020-06-26 15:14, Jorn Vernee wrote: Hi, https://bugs.openjdk.java.net/browse/JDK-8248135 added --enable-preview to the javac options when building micro benchmarks. We should also add it to the set of default VM arguments passed to the microbenchmark jar so it doesn't need to be passed manually. If --enable-preview is not passed, the microbenchmarks can not be run (even the ones that don't use preview features). Since the class files have a modified minor version due to building with --enable-preview, the VM must also be started with --enable-preview in order to be able to load the classes. In the absence of --enable-preview, for instance the following error will occur: java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/jdk/incubator/foreign/generated/CallOverhead_panama_args10_jmhTest (class file version 60.65535). Try running with '--enable-preview' at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1016) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:151) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:825) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:723) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:646) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:604) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:168) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:377) at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:233) at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:139) at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:76) at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) Please review the patch attached inline at [1]. Testing: running a microbenchmark without passing '--enable-preview' manually and confirming that it doesn't fail to load the classes. Thanks, Jorn [1] : diff --git a/make/RunTests.gmk b/make/RunTests.gmk index 721bb827639..59911d89e9f 100644 --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -691,7 +691,7 @@ define SetupRunMicroTestBody endif # Set library path for native dependencies - $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native + $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native --enable-preview ifneq ($$(MICRO_VM_OPTIONS)$$(MICRO_JAVA_OPTIONS), ) $1_JMH_JVM_ARGS += $$(MICRO_VM_OPTIONS) $$(MICRO_JAVA_OPTIONS)
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Patch looks fine (although you might want to update the comment). It's more concerning that I didn't catch this (seems all tests of mine were with --enable-preview), and we'll still inconvenience users who need to run the jar file directly. So it seems we should consider fixing so that only those benchmarks that actually need --enable-preview are built with that flag. /Claes On 2020-06-27 00:21, Jorn Vernee wrote: Forgot to attach the JBS link: https://bugs.openjdk.java.net/browse/JDK-8248429 Jorn On 27/06/2020 00:14, Jorn Vernee wrote: Hi, https://bugs.openjdk.java.net/browse/JDK-8248135 added --enable-preview to the javac options when building micro benchmarks. We should also add it to the set of default VM arguments passed to the microbenchmark jar so it doesn't need to be passed manually. If --enable-preview is not passed, the microbenchmarks can not be run (even the ones that don't use preview features). Since the class files have a modified minor version due to building with --enable-preview, the VM must also be started with --enable-preview in order to be able to load the classes. In the absence of --enable-preview, for instance the following error will occur: java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/jdk/incubator/foreign/generated/CallOverhead_panama_args10_jmhTest (class file version 60.65535). Try running with '--enable-preview' at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1016) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:151) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:825) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:723) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:646) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:604) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:168) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:377) at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:233) at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:139) at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:76) at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) Please review the patch attached inline at [1]. Testing: running a microbenchmark without passing '--enable-preview' manually and confirming that it doesn't fail to load the classes. Thanks, Jorn [1] : diff --git a/make/RunTests.gmk b/make/RunTests.gmk index 721bb827639..59911d89e9f 100644 --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -691,7 +691,7 @@ define SetupRunMicroTestBody endif # Set library path for native dependencies - $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native + $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native --enable-preview ifneq ($$(MICRO_VM_OPTIONS)$$(MICRO_JAVA_OPTIONS), ) $1_JMH_JVM_ARGS += $$(MICRO_VM_OPTIONS) $$(MICRO_JAVA_OPTIONS)
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Forgot to attach the JBS link: https://bugs.openjdk.java.net/browse/JDK-8248429 Jorn On 27/06/2020 00:14, Jorn Vernee wrote: Hi, https://bugs.openjdk.java.net/browse/JDK-8248135 added --enable-preview to the javac options when building micro benchmarks. We should also add it to the set of default VM arguments passed to the microbenchmark jar so it doesn't need to be passed manually. If --enable-preview is not passed, the microbenchmarks can not be run (even the ones that don't use preview features). Since the class files have a modified minor version due to building with --enable-preview, the VM must also be started with --enable-preview in order to be able to load the classes. In the absence of --enable-preview, for instance the following error will occur: java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/jdk/incubator/foreign/generated/CallOverhead_panama_args10_jmhTest (class file version 60.65535). Try running with '--enable-preview' at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1016) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:151) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:825) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:723) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:646) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:604) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:168) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:377) at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:233) at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:139) at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:76) at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) Please review the patch attached inline at [1]. Testing: running a microbenchmark without passing '--enable-preview' manually and confirming that it doesn't fail to load the classes. Thanks, Jorn [1] : diff --git a/make/RunTests.gmk b/make/RunTests.gmk index 721bb827639..59911d89e9f 100644 --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -691,7 +691,7 @@ define SetupRunMicroTestBody endif # Set library path for native dependencies - $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native + $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native --enable-preview ifneq ($$(MICRO_VM_OPTIONS)$$(MICRO_JAVA_OPTIONS), ) $1_JMH_JVM_ARGS += $$(MICRO_VM_OPTIONS) $$(MICRO_JAVA_OPTIONS)
RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Hi, https://bugs.openjdk.java.net/browse/JDK-8248135 added --enable-preview to the javac options when building micro benchmarks. We should also add it to the set of default VM arguments passed to the microbenchmark jar so it doesn't need to be passed manually. If --enable-preview is not passed, the microbenchmarks can not be run (even the ones that don't use preview features). Since the class files have a modified minor version due to building with --enable-preview, the VM must also be started with --enable-preview in order to be able to load the classes. In the absence of --enable-preview, for instance the following error will occur: java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/jdk/incubator/foreign/generated/CallOverhead_panama_args10_jmhTest (class file version 60.65535). Try running with '--enable-preview' at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1016) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:151) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:825) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:723) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:646) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:604) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:168) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:377) at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:233) at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:139) at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:76) at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) Please review the patch attached inline at [1]. Testing: running a microbenchmark without passing '--enable-preview' manually and confirming that it doesn't fail to load the classes. Thanks, Jorn [1] : diff --git a/make/RunTests.gmk b/make/RunTests.gmk index 721bb827639..59911d89e9f 100644 --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -691,7 +691,7 @@ define SetupRunMicroTestBody endif # Set library path for native dependencies - $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native + $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native --enable-preview ifneq ($$(MICRO_VM_OPTIONS)$$(MICRO_JAVA_OPTIONS), ) $1_JMH_JVM_ARGS += $$(MICRO_VM_OPTIONS) $$(MICRO_JAVA_OPTIONS)
