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/<config>/support/test/micro and
build/<config>/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.<init>(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)
