RFR: 8232118 - Add JVM option to enable JVMCI compilers in product mode

[Bob Vandette]

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>


> On Oct 24, 2019, at 1:22 AM, David Holmes <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
>> WEBREV: 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
>> Bob.