RFR: 8232118 - Add JVM option to enable JVMCI compilers in product mode
Doug Simon
doug.simon at oracle.com
Mon Oct 28 21:21:42 UTC 2019
> On 28 Oct 2019, at 21:50, Bob Vandette <bob.vandette at oracle.com> wrote:
>
>
>> On Oct 28, 2019, at 4:40 PM, Doug Simon <doug.simon at oracle.com <mailto:doug.simon at oracle.com>> wrote:
>>
>>
>>
>>> On 28 Oct 2019, at 18:18, Vladimir Kozlov <vladimir.kozlov at oracle.com <mailto:vladimir.kozlov at oracle.com>> wrote:
>>>
>>> On 10/28/19 8:37 AM, Bob Vandette wrote:
>>>>> On Oct 25, 2019, at 6:57 AM, Doug Simon <doug.simon at oracle.com <mailto:doug.simon at oracle.com>> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> On 25 Oct 2019, at 02:06, David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
>>>>>>
>>>>>> On 25/10/2019 1:27 am, Bob Vandette wrote:
>>>>>>> Thanks for the review. See comments below.
>>>>>>> Updated webrev: http://cr.openjdk.java.net/~bobv/8232118/webrev.02 <http://cr.openjdk.java.net/~bobv/8232118/webrev.02>
>>>>>>
>>>>>> That all seems fine.
>>>>>>
>>>>>> Minor nit in arguments.cpp:
>>>>>>
>>>>>> + "Unable to enable jvmci in product mode");
>>>>>>
>>>>>> s/jvmci/JVMCI/
>>>>>>
>>>>>> One follow up ... you don't modify all of the JVMCI flags. The following are omitted:
>>>>>>
>>>>>> BootstrapJVMCI
>>>>>> JVMCIHostThreads
>>>>>> JVMCITraceLevel
>>>>>> MethodProfileWidth
>>>>>> PrintBootstrap
>>>>>
>>>>> These are all diagnostic flags. Bob, can EnableJVMCIProduct make them into actual diagnostic flags?
>>>
>>> Doug, the only question here if you want customers to use them for some diagnostic logging? Then we should consider converting them to diagnostic. If it is only for our internal debugging testing lets leave them experimental.
>>>
>>> We have JVMCI tests which use BootstrapJVMCI with UnlockExperimentalVMOptions. If we change the flag's type with EnableJVMCIProduct we need to update tests.
>>
>> We have a similar issue in GraalVM.
>>
>>>> If they are really diagnostic flags, then they should be declared that way.
>>>
>>> We can't declare them diagnostic in JDK yet until Graal become product.
>>
>> I thought the whole point of EnableJVMCIProduct is to change JVMCI flags to something other than experimental. Not sure why it’s ok to change some to “product” but not others to “diagnostic”.
>
> Vladimir is responding to my comment about making these flags diagnostic independent of EnableJVMCIProduct.
>
>>
>>>
>>>> Leaving them as is still allows you to use them, they just need to be unlocked with the Experimental flag.
>>>> I’d prefer leaving leave them alone and let the compiler team decide what they want to do if/when JVMCI is no
>>>> longer experimental.
>>>
>>> I almost agree here but if they really needed to diagnose issues on customer side we should consider converting them as Doug asked.
>>
>> Since we can also ask customers to enable experimental options for diagnostic purposes, its ok to leave as experimental.
>
> We could do this but wouldn’t we have some confusion with support folks who might read the source and see that this flag is
> declared experimental but since EnableJVMCIProduct is hidden in the jlink’d module file, they won’t know which flag to use
> if we changed it under the covers to be a diagnostic one?
Won’t they have the same problem with any of the JVMCI flags that become product flags as a result of EnableJVMCIProduct? I have the feeling that I’m not understanding your question correctly.
-Doug
>
>>
>> -Doug
>>
>>>> Bob.
>>>>>
>>>>> -Doug
>>>>>
>>>>>>
>>>>>>
>>>>>>>> On Oct 24, 2019, at 1:22 AM, David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com> <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com>>> wrote:
>>>>>>>>
>>>>>>>> Hi Bob,
>>>>>>>>
>>>>>>>> On 24/10/2019 1:13 am, Bob Vandette wrote:
>>>>>>>>> Please review this RFE that adds a new JVM option "-XX:+EnableJVMCIProduct" which will allow JVMCI to be used as the default compiler,
>>>>>>>>> and alter a collection of JVM options to be product flags rather than experimental flags.
>>>>>>>>> EnableJVMCIProduct is an experimental flag since the default JVMCI running mode is still experimental. A vendor wishing to support their JVMCI compiler as the default can enable JVMCI by default by specifying this new -XX:+EnableJVMCIProduct flag after the -XX:+UnlockExperimentalVMOptions flag. This flag is especially useful when added to a JDK runtime generated with the new jlink --add-options plugin (see JDK-8232080 [1]). This allows JVMCI based compilers to be used, by default, without the user having to specify any options.
>>>>>>>>
>>>>>>>> Okay, so there are few things happening here so let me step through them. The desire is to enable an experimental compiler by default, but not have to run with all experimental options unlocked (which I assume is just to try and protect the end user from their own mistakes?). So the way this would work is that the command-line would effectively be:
>>>>>>>>
>>>>>>>> -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCIProducts --XX:-UnlockExperimentalVMOptions
>>>>>>>>
>>>>>>>> when EnableJVMCIProducts is processed (and is true) it will force on a couple of key JVMCI related flags, and it will turn off the flag-is-experimental bit on all the JVMCI flags. The latter being desirable because otherwise, with -XX:-UnlockExperimentalVMOptions in play, all those other JVMCI flags would be hidden and you want them to be visible to introspection (like JVM TI, Dcmd) - is that correct?
>>>>>>> Yes, that’s correct.
>>>>>>>>
>>>>>>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8232118 <https://bugs.openjdk.java.net/browse/JDK-8232118>
>>>>>>>>> WEBREV: http://cr.openjdk.java.net/~bobv/8232118/webrev.01 <http://cr.openjdk.java.net/~bobv/8232118/webrev.01>
>>>>>>>>
>>>>>>>> Looking at the code in detail I have a few comments.
>>>>>>>>
>>>>>>>> src/hotspot/share/jvmci/jvmci_globals.cpp
>>>>>>>>
>>>>>>>> if (UseJVMCICompiler) {
>>>>>>>> + if (FLAG_IS_DEFAULT(UseJVMCINativeLibrary) && !UseJVMCINativeLibrary) {
>>>>>>>> + char path[JVM_MAXPATHLEN];
>>>>>>>> + if (os::dll_locate_lib(path, sizeof(path), Arguments::get_dll_dir(), JVMCI_SHARED_LIBRARY_NAME)) {
>>>>>>>> + // If a JVMCI native library is present,
>>>>>>>> + // we enable UseJVMCINativeLibrary by default.
>>>>>>>> + FLAG_SET_DEFAULT(UseJVMCINativeLibrary, true);
>>>>>>>> + }
>>>>>>>> + }
>>>>>>>>
>>>>>>>> This change will affect use of JVMCI independent of whether EnableJVMCIProduct is set - was that intentional?
>>>>>>> Yes, the compiler team is ok with this additional functionality being available when UseJVMCICompiler is true
>>>>>>> independent of EnableJVMCIProduct. I’ve updated the RFE text to be clearer that this change is independent of
>>>>>>> the new flag.
>>>>>>>>
>>>>>>>> In JVMCIGlobals::enable_jvmci_product_mode, if we return false then argument processing will return JNI_ERR and the loading of the VM will terminate. It is not clear to me that the user would get any useful information regarding what failed. That said I can't really see what could actually fail in that code ... but perhaps a warning would be useful?
>>>>>>> Ok.
>>>>>>>>
>>>>>>>> I see where you clear the experimental bit, but as far as I can see that won't turn the flag into a product flag but will leave it uncategorised. ?? That probably works for you as you just need to ensure is_unlocked() returns true, but it does leave the flag in a strange state IMO.
>>>>>>> Thanks for that find, I assumed !experimental was product. I’ll add a set_product function and call it when I remove the experimental bit.
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> src/hotspot/share/runtime/arguments.cpp
>>>>>>>>
>>>>>>>> + #if INCLUDE_JVMCI
>>>>>>>> + } else if (match_option(option, "-XX:+EnableJVMCIProduct")) {
>>>>>>>> + JVMFlag *jvmciFlag = JVMFlag::find_flag("EnableJVMCIProduct");
>>>>>>>> + // Allow this flag if it has been unlocked.
>>>>>>>> + if (jvmciFlag != NULL && jvmciFlag->is_unlocked()) {
>>>>>>>> + if (!JVMCIGlobals::enable_jvmci_product_mode(origin)) {
>>>>>>>> + return JNI_ERR;
>>>>>>>> + }
>>>>>>>> + } else if (match_option(option, "-XX:", &tail)) {
>>>>>>>> + if (!process_argument(tail, args->ignoreUnrecognized, origin)) {
>>>>>>>> + return JNI_EINVAL;
>>>>>>>> + }
>>>>>>>> + }
>>>>>>>>
>>>>>>>> It took me quite a while to understand that the point of
>>>>>>>>
>>>>>>>> + } else if (match_option(option, "-XX:", &tail)) {
>>>>>>>>
>>>>>>>> is purely to isolate the "EnableJVMCIProduct" part so that it can be processed by process_argument and report the expected error message that the flag has not been unlocked. I think it would be much clearer to just do that explicitly:
>>>>>>>>
>>>>>>>> + if (jvmciFlag != NULL && jvmciFlag->is_unlocked()) {
>>>>>>>> + if (!JVMCIGlobals::enable_jvmci_product_mode(origin)) {
>>>>>>>> + return JNI_ERR;
>>>>>>>> + }
>>>>>>>> + }
>>>>>>>> + // The flag was locked so process normally to report that error
>>>>>>>> + else if (!process_argument("EnableJVMCIProduct", args->ignoreUnrecognized, origin)) {
>>>>>>>> + return JNI_EINVAL;
>>>>>>>> + }
>>>>>>>>
>>>>>>>>
>>>>>>> Ok.
>>>>>>>> Also the whole else-if block you added appears to be missing the final } to close it. ??
>>>>>>> Its on the next line after the #endif. This is how all the if cases are structures.
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> src/hotspot/share/runtime/flags/jvmFlag.cpp
>>>>>>>>
>>>>>>>> + case RESOURCE:
>>>>>>>> + st->print("resource"); break;
>>>>>>>>
>>>>>>>> src/hotspot/share/runtime/flags/jvmFlag.hpp
>>>>>>>>
>>>>>>>> ! RESOURCE = 7,
>>>>>>>> ! INTERNAL = 8,
>>>>>>>>
>>>>>>>> I think the RESOURCE changes are part of the other RFE. ??
>>>>>>> Correct, these leaked in from another RFE and will be removed from my push.
>>>>>>> Thanks,
>>>>>>> Bob.
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
>>>>>>>>
>>>>>>>>> [1] Related RFE: https://bugs.openjdk.java.net/browse/JDK-8232080 <https://bugs.openjdk.java.net/browse/JDK-8232080>
>>>>>>>>> Bob.
>>
>
More information about the hotspot-runtime-dev
mailing list