RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks

Claes Redestad claes.redestad at oracle.com
Tue Jun 30 16:19:35 UTC 2020 


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/<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)
>>>>>>>>
>>>
>

More information about the build-dev mailing list