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

[Claes Redestad]

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