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

Tue Jun 30 14:15:20 UTC 2020


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