[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR: 8232118 - Add JVM option to enable JVMCI compilers in product mode
From: Bob Vandette <bob.vandette () oracle ! com>
Date: 2019-10-28 21:38:33
Message-ID: 3203D054-4659-4A80-A01B-352EF85F1E43 () oracle ! com
[Download RAW message or body]
> On Oct 28, 2019, at 5:21 PM, Doug Simon <doug.simon@oracle.com> wrote:
>
>
>
> > On 28 Oct 2019, at 21:50, Bob Vandette <bob.vandette@oracle.com \
> > <mailto:bob.vandette@oracle.com>> wrote:
> >
> > > On Oct 28, 2019, at 4:40 PM, Doug Simon <doug.simon@oracle.com \
> > > <mailto:doug.simon@oracle.com>> wrote:
> > >
> > >
> > > > On 28 Oct 2019, at 18:18, Vladimir Kozlov <vladimir.kozlov@oracle.com \
> > > > <mailto:vladimir.kozlov@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@oracle.com \
> > > > > > <mailto:doug.simon@oracle.com>> wrote:
> > > > > >
> > > > > >
> > > > > > > On 25 Oct 2019, at 02:06, David Holmes <david.holmes@oracle.com \
> > > > > > > <mailto:david.holmes@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.
The difference with Product flags is that they don't need any unlocking. In any \
case, we'll stick with experimental for these few flags for now and revisit this \
later.
Bob.
> -Doug
>
> >
> > >
> > > -Doug
> > >
> > > > > Bob.
> > > > > >
> > > > > > -Doug
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > > On Oct 24, 2019, at 1:22 AM, David Holmes <david.holmes@oracle.com \
> > > > > > > > > <mailto:david.holmes@oracle.com> <mailto:david.holmes@oracle.com \
> > > > > > > > > <mailto:david.holmes@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 \
> > > > > > > > > > <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.
> > >
> >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic