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

[David Holmes]

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?

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

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?

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.

---

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;
+       }


Also the whole else-if block you added appears to be missing the final } 
to close it. ??

---

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. ??

---

Thanks,
David
-----


> [1] Related RFE: https://bugs.openjdk.java.net/browse/JDK-8232080
> 
> Bob.
> 
>