8309136: [JVMCI] add -XX:+UseGraalJIT flag #14231

dougxc · 2023-05-30T22:31:13Z

Use of the Graal-based JIT in OpenJDK currently requires the following flag: -XX:+EnableJVMCIProduct

This has no direct association with Graal. If the JDK image happens to include a non-Graal JVMCI implementation, it will be automatically selected. This would come as a surprise to users who equate JVMCI with Graal.

This PR introduces a new flag, -XX:+UseGraalJIT to address these shortcomings. It is an alias for -XX:+EnableJVMCIProduct -Djvmci.Compiler=graal.

When -XX:+UseGraalJIT is specified, the VM fails fast at startup if there is a non-Graal JVMCI implementation or no JVMCI implementation in the JDK image.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change requires CSR request JDK-8309137 to be approved
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issues

JDK-8309136: [JVMCI] add -XX:+UseGraalJIT flag
JDK-8309137: [JVMCI] add -XX:+UseGraalJIT flag (CSR)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14231/head:pull/14231
$ git checkout pull/14231

Update a local copy of the PR:
$ git checkout pull/14231
$ git pull https://git.openjdk.org/jdk.git pull/14231/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14231

View PR using the GUI difftool:
$ git pr show -t 14231

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14231.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-30T22:32:25Z

👋 Welcome back dnsimon! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-05-30T22:34:22Z

@dougxc The following labels will be automatically applied to this pull request:

graal
hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-05-30T22:38:03Z

Webrevs

dholmes-ora

A couple of minor items but otherwise the flag handling seems fine (though not what was linked in the CSR request).

Thanks.

dholmes-ora · 2023-05-31T07:23:45Z

src/hotspot/share/jvmci/jvmci_globals.hpp

  product(bool, EnableJVMCI, false, EXPERIMENTAL,                           \
          "Enable JVMCI")                                                   \
                                                                            \
+  product(bool, UseGraalJIT, false,                                         \


This should be EXPERIMENTAL

dholmes-ora · 2023-05-31T07:24:01Z

src/hotspot/share/jvmci/jvmci_globals.hpp

          "Enable JVMCI")                                                   \
                                                                            \
+  product(bool, UseGraalJIT, false,                                         \
+          "Select the Graal JVMCI compiler. This is a macro for: "          \


s/a macro/an alias/ please

dougxc · 2023-05-31T08:06:21Z

though not what was linked in the CSR request

Would you mind expanding on the discrepancy? I'd like to make it consistent.

dholmes-ora · 2023-05-31T08:19:20Z

In the CSR request when asked about the response when the run-time image does not include the Graal JIT you pointed to this Java code:

jdk/src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotJVMCICompilerConfig.java

Line 112 in 927a9ed

    
           throw runtime.exitHotSpotWithMessage(1, "JVMCI compiler '%s' specified by %s not found%n", compilerName, compPropertyName);

but it seems to me that the logic in arguments.cpp handles the case where Graal is not available. Or are there different levels of "availability" here?

dougxc · 2023-05-31T08:27:59Z

The logic in arguments.cpp only ensures the value of jvmci.Compiler is consistent with -XX:+UseGraalJIT. Actually checking the availability of Graal can only be done in the code I linked to and it's only done on the first Graal JIT compilation (JVMCI compiler initialization is lazy to reduce impact to VM startup time).

vnkozlov

What about UseJVMCICompiler flag? A lot of code in HotSpot is guarded by this flag.

dougxc · 2023-05-31T20:08:10Z

What about UseJVMCICompiler flag? A lot of code in HotSpot is guarded by this flag.

UseGraalJIT is an alias for EnableJVMCIProduct and the latter sets UseJVMCICompiler to true.

vnkozlov

Update Copyright year in test.

vnkozlov · 2023-05-31T21:58:38Z

src/hotspot/share/runtime/arguments.cpp

+      if (match_option(option, "-XX:+UseGraalJIT")) {
+        const char* jvmci_compiler = get_property("jvmci.Compiler");
+        if (jvmci_compiler != nullptr) {
+          if (strcmp(jvmci_compiler, "graal") != 0) {


You should not use strcmp - use strncmp.

vnkozlov · 2023-05-31T22:09:12Z

src/hotspot/share/runtime/arguments.cpp

        jio_fprintf(defaultStream::error_stream(),
-                  "-XX:-EnableJVMCIProduct cannot come after -XX:+EnableJVMCIProduct\n");
+                  "-XX:-EnableJVMCIProduct or -XX:-UseGraalJIT cannot come after -XX:+EnableJVMCIProduct or -XX:+UseGraalJIT\n");
        return JNI_EINVAL;


Why this restriction? Usually latest specified flag wins. May be comment with explanation.
Also you did not check for if (UseGraalJIT).
What about the case -XX:+EnableJVMCIProduct -XX:-UseGraalJIT?
I would expect this kind of checks done in JVMCIGlobals::check_jvmci_flags_are_consistent()

Why this restriction? Usually latest specified flag wins. May be comment with explanation.

This is a pre-existing restriction on EnableJVMCIProduct and since UseGraalJIT is an alias for EnableJVMCIProduct, it must abide by the same restrictions. For whatever reason, the effect of +EnableJVMCIProduct cannot be undone and so the same must hold for +UseGraalJIT.

Also you did not check for if (UseGraalJIT).

This check is not necessary as UseGraalJIT is an alias. When EnableJVMCIProduct is set to true, UseGraalJIT will also be set to true. Stating this now, I see that I missed it: 430b7a5

What about the case -XX:+EnableJVMCIProduct -XX:-UseGraalJIT?

That results in:

-XX:-EnableJVMCIProduct or -XX:-UseGraalJIT cannot come after -XX:+EnableJVMCIProduct or -XX:+UseGraalJIT Error: Could not create the Java Virtual Machine. Error: A fatal exception has occurred. Program will exit.

which is what is expected.

vnkozlov · 2023-05-31T22:11:21Z

test/hotspot/jtreg/compiler/jvmci/TestEnableJVMCIProduct.java

            new Expectation("UseJVMCICompiler", "false", "default"));
        test("-XX:+EnableJVMCIProduct",
-            new Expectation("EnableJVMCIProduct", "true", "command line"),
+            new Expectation("EnableJVMCIProduct", "true", "(?:command line|jimage)"),


What "(?:command line|jimage)" means? Also | hard to see.

That's a standard Java regular expression and it means the value is expected have been set on the command line or by the VM options read from the jimage (see JDK-8232080).

…er -Xint

dholmes-ora · 2023-05-31T23:12:34Z

Actually checking the availability of Graal can only be done in the code I linked to and it's only done on the first Graal JIT compilation (JVMCI compiler initialization is lazy to reduce impact to VM startup time).

Are you saying that the application will start executing quite happily in interpreted mode and then only abort when the JIT first kicks in? That would be a very bad thing to do unless you can guarantee the jit will kick in extremely early during VM init sequence.

…erimentalVMOptions

vnkozlov · 2023-05-31T23:21:16Z

Actually checking the availability of Graal can only be done in the code I linked to and it's only done on the first Graal JIT compilation (JVMCI compiler initialization is lazy to reduce impact to VM startup time).

Are you saying that the application will start executing quite happily in interpreted mode and then only abort when the JIT first kicks in? That would be a very bad thing to do unless you can guarantee the jit will kick in extremely early during VM init sequence.

I assume this is required only for Java Graal. For libgraal you can check library presence at the VM startup. Or I am wrong?

dougxc · 2023-05-31T23:29:57Z

Actually checking the availability of Graal can only be done in the code I linked to and it's only done on the first Graal JIT compilation (JVMCI compiler initialization is lazy to reduce impact to VM startup time).

Are you saying that the application will start executing quite happily in interpreted mode and then only abort when the JIT first kicks in? That would be a very bad thing to do unless you can guarantee the jit will kick in extremely early during VM init sequence.

I assume this is required only for Java Graal. For libgraal you can check library presence at the VM startup. Or I am wrong?

JVMCI compiler initialization is always lazy. It has been this way since the initial JVMCI implementation. Even libgraal initialization will impact VM startup. The first top tier compilation usually happens within the first 100 ms of VM execution.

vnkozlov

Looks good now. You need second approval.

dholmes-ora

The changes here seem fine, but I remain concerned about the compiler detection logic that presently exists. I can run trivial "programs" using -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCIProduct -Djvmci.Compiler=graal and there is no error. That means that JIT compilation is not kicking in during VM init and that application code is being executed prior to the VM aborting! That is very wrong behaviour IMO, but it is pre-existing so would need to be addressed in separate bug.

dougxc · 2023-06-01T08:15:44Z

Thanks for input David. I agree that it's best to open a new JBS issue to discuss concerns about lazy JVMCI compiler initialization.

…8309136

openjdk · 2023-06-05T16:35:17Z

@dougxc This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8309136: [JVMCI] add -XX:+UseGraalJIT flag

Reviewed-by: dholmes, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 23 new commits pushed to the master branch:

5cd8af7: 8308726: RISC-V: avoid unnecessary slli in the vectorized arraycopy stubs for bytes
80232b7: 8308969: make test-prebuilt doesn't return the correct exit code
6d511f1: 8309129: AArch64: guarantee(T != T2S) failed: "incorrect arrangement" after JDK-8307795
9be5769: 8291966: SwitchBootstrap.typeSwitch could be faster
db0857d: 8306578: Report error if no label given in @see and {@link} when no default is available
8c9d21e: 8296920: Regression Test DialogOrient.java fails on MacOS
ac41c03: 8303465: KeyStore of type KeychainStore, provider Apple does not show all trusted certificates
11fb5b2: 8309104: [JVMCI] compiler/unsafe/UnsafeGetStableArrayElement test asserts wrong values with Graal
a02d800: 8309405: RISC-V: is_deopt may produce unaligned memory read
3fa776d: 8302673: [SuperWord] MaxReduction and MinReduction should vectorize for int
... and 13 more: https://git.openjdk.org/jdk/compare/931913fbb299fbed7485ab8229100e6e56d8bada...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

dougxc · 2023-06-05T20:47:06Z

Thanks to all the reviewers.

/integrate

openjdk · 2023-06-05T20:47:58Z

Going to push as commit b3c9d67.
Since your change was applied there have been 28 commits pushed to the master branch:

98b53c0: 8306112: Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview)
e970ddb: 8309170: CDS archive heap is always relocated for larger heap
4b8922f: 8308842: Consolidate exceptions thrown from Class-File API
2b38343: 8309416: Misstatement in semantics of methods in javax.lang.model.ElementFilter
73352b6: 8280994: [XWayland] Drag and Drop does not work in java -> wayland app direction
5cd8af7: 8308726: RISC-V: avoid unnecessary slli in the vectorized arraycopy stubs for bytes
80232b7: 8308969: make test-prebuilt doesn't return the correct exit code
6d511f1: 8309129: AArch64: guarantee(T != T2S) failed: "incorrect arrangement" after JDK-8307795
9be5769: 8291966: SwitchBootstrap.typeSwitch could be faster
db0857d: 8306578: Report error if no label given in @see and {@link} when no default is available
... and 18 more: https://git.openjdk.org/jdk/compare/931913fbb299fbed7485ab8229100e6e56d8bada...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-05T20:48:09Z

@dougxc Pushed as commit b3c9d67.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

ysramakrishna · 2024-01-09T02:23:36Z

Thanks for input David. I agree that it's best to open a new JBS issue to discuss concerns about lazy JVMCI compiler initialization.

@dougxc : was the "new JBS issue" opened? If so, can it be linked here and in https://bugs.openjdk.org/browse/JDK-8309136.

dougxc · 2024-01-09T04:11:46Z

@ysramakrishna I'm not aware of an issue being opened. I'd prefer to avoid opening an issue until we better understand where the concern results in a real problem in practice. If you have something along these lines, feel free to open the issue.

add UseGraalJIT VM flag

f9b3538

dougxc marked this pull request as ready for review May 30, 2023 22:32

openjdk bot added csr Pull request needs approved CSR before integration rfr Pull request is ready for review labels May 30, 2023

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels May 30, 2023

dholmes-ora suggested changes May 31, 2023

View reviewed changes

review based fixes

235075c

vnkozlov reviewed May 31, 2023

View reviewed changes

added missing test of UseJVMCICompiler when adjusting JVMCI flags und…

2d1a1e9

…er -Xint

dougxc added 4 commits June 1, 2023 01:13

set UseGraalJIT value in enable_jvmci_product_mode

430b7a5

fix date in copyright header

578b753

use strncmp instead of strcmp

60370c5

improve error message when UseGraalJIT is used without -XX:+UnlockExp…

28f0a8b

…erimentalVMOptions

vnkozlov approved these changes Jun 1, 2023

View reviewed changes

dholmes-ora approved these changes Jun 1, 2023

View reviewed changes

[skip-ci] Merge remote-tracking branch 'openjdk-jdk/master' into JDK-…

bcbab07

…8309136

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Jun 5, 2023

openjdk bot added the integrated Pull request has been integrated label Jun 5, 2023

openjdk bot closed this Jun 5, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 5, 2023

8309136: [JVMCI] add -XX:+UseGraalJIT flag #14231

8309136: [JVMCI] add -XX:+UseGraalJIT flag #14231

Uh oh!

Conversation

dougxc commented May 30, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 30, 2023

Uh oh!

openjdk bot commented May 30, 2023

Uh oh!

mlbridge bot commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora May 31, 2023

Choose a reason for hiding this comment

Uh oh!

dholmes-ora May 31, 2023

Choose a reason for hiding this comment

Uh oh!

dougxc commented May 31, 2023

Uh oh!

dholmes-ora commented May 31, 2023

Uh oh!

dougxc commented May 31, 2023

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

dougxc commented May 31, 2023

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov May 31, 2023

Choose a reason for hiding this comment

Uh oh!

vnkozlov May 31, 2023

Choose a reason for hiding this comment

Uh oh!

dougxc May 31, 2023

Choose a reason for hiding this comment

Uh oh!

vnkozlov May 31, 2023

Choose a reason for hiding this comment

Uh oh!

dougxc May 31, 2023

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented May 31, 2023

Uh oh!

vnkozlov commented May 31, 2023

Uh oh!

dougxc commented May 31, 2023

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dougxc commented Jun 1, 2023

Uh oh!

openjdk bot commented Jun 5, 2023

Uh oh!

dougxc commented Jun 5, 2023

Uh oh!

openjdk bot commented Jun 5, 2023

Uh oh!

openjdk bot commented Jun 5, 2023

Uh oh!

dougxc commented May 30, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 30, 2023 •

edited

Loading