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

[David Holmes]

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

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

Thanks,
David



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