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

Magnus Ihse Bursie magnus.ihse.bursie at oracle.com
Tue Jun 30 15:16:29 UTC 2020 


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