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

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


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

More information about the build-dev mailing list