8345826: Do not automatically resolve jdk.internal.vm.ci when libgraal is used #25240

dougxc · 2025-05-14T22:00:30Z

The EnableJVMCI flag currently serves 2 purposes:

Guards VM code (example).
Adds jdk.internal.vm.ci to the root module set.

This PR changes nothing about the first point.

On the second point, to use the jdk.internal.vm.ci module when libgraal is enabled, -XX:+EnableJVMCI must be explicitly specified to the launcher (as opposed to being true as a result of -XX:+UseJVMCICompiler or -XX:+EnableJVMCIProduct). Alternatively, --add-modules=jdk.internal.vm.ci can be specified - it has the same semantics as -XX:+EnableJVMCI.
If libgraal is not enabled, +EnableJVMCI will continue to add jdk.internal.vm.ci to the root module set.

The primary motivation is to make use of libgraal compatible with -XX:+AOTClassLinking. This flag relies on the root module set archive created in a training run. If the root module set is different in the production run, the AOTClassLinking optimizations are disabled. As jdk.internal.vm.ci is not resolved in the training run, it must not be resolved in production run. As such, -XX:+EnableJVMCI must not cause resolution of jdk.internal.vm.ci, otherwise libgraal will not have the startup advantages of AOTClassLinking.

Graal adaption PR: oracle/graal#11212

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8345826: Do not automatically resolve jdk.internal.vm.ci when libgraal is used (Enhancement - P3)

Reviewers

Ioi Lam (@iklam - Reviewer)
Tom Rodriguez (@tkrodriguez - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25240

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-05-14T22:01:17Z

👋 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 · 2025-05-14T22:01:43Z

@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:

8345826: Do not automatically resolve jdk.internal.vm.ci when libgraal is used

Reviewed-by: iklam, never, 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 226 new commits pushed to the master branch:

6546de1: 8357478: Fix copyright header in src/jdk.jpackage/share/classes/jdk/jpackage/internal/AppImageDesc.java
275cfd3: 8357280: (bf) Remove @requires tags from java/nio/Buffer/LimitDirectMemory[NegativeTest].java
254713a: 8338833: Error on reference not found for a snippet target
... and 223 more: https://git.openjdk.org/jdk/compare/52a5583d691388f833c3aeb56ce92cbfb5d61274...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.

openjdk · 2025-05-14T22:02:23Z

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

tkrodriguez · 2025-05-15T16:10:12Z

I found your explanation quite confusing, but the bug title is actually the most clear description of the fix. Basically libjvmci doesn't require the existence of jdk.internal.vm.ci on the HotSpot side since it has effectively compiled that into itself. So we are decoupling the ability to use JVMCI from the presence of the JVMCI module. A short comment along these lines in at least your changes in check_vm_args_consistency would be helpful I think.

I do find it confusing that we are explicitly passing --add-modules=jdk.internal.vm.ci in a bunch of the tests. Is that now necessary or are you just exercising the alternate ways of enabling JVMCI?

dougxc · 2025-05-15T21:42:06Z

I found your explanation quite confusing, but the bug title is actually the most clear description of the fix. Basically libjvmci doesn't require the existence of jdk.internal.vm.ci on the HotSpot side since it has effectively compiled that into itself. So we are decoupling the ability to use JVMCI from the presence of the JVMCI module. A short comment along these lines in at least your changes in check_vm_args_consistency would be helpful I think.

I added the requested comment and tried to clarify the PR description. Let me know if clarification is needed.

I do find it confusing that we are explicitly passing --add-modules=jdk.internal.vm.ci in a bunch of the tests. Is that now necessary or are you just exercising the alternate ways of enabling JVMCI?

Without that option, the module will be missing and without the fail-fast check in check_vm_args_consistency you would get an error such as:

Uncaught exception at src/hotspot/share/jvmci/jvmciRuntime.cpp:1433
java.lang.NoClassDefFoundError: jdk/vm/ci/code/Architecture
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (jvmciRuntime.cpp:1636), pid=1979, tid=9731
#  fatal error: Fatal JVMCI exception (see JVMCI Events for stack trace): Uncaught exception at src/hotspot/share/jvmci/jvmciRuntime.cpp:1433
#

mlbridge · 2025-05-15T21:54:17Z

Webrevs

vnkozlov · 2025-05-15T22:17:42Z

Basically libjvmci doesn't require the existence of jdk.internal.vm.ci on the HotSpot side since it has effectively compiled that into itself. So we are decoupling the ability to use JVMCI from the presence of the JVMCI module.

That should be in PR and RFE (JBS) Descriptions! This was my main question about filed REF.

vnkozlov · 2025-05-15T22:24:19Z

src/hotspot/share/runtime/arguments.cpp

+    // compiled into the libjvmci image itself. Without libjvmci, there
+    // is no other representation of the jdk.internal.vm.ci module
+    // so it needs to be added to the root module set.
+    if (ClassLoader::is_module_observable("jdk.internal.vm.ci") && !UseJVMCINativeLibrary && !_jvmci_module_added) {


In which case ClassLoader::is_module_observable("jdk.internal.vm.ci") == true when _jvmci_module_added == false?
I assume this code is executed after check command line for --add-modules=jdk.internal.vm.ci.

The documentation for is_module_observable is:

// Determines if the named module is present in the // modules jimage file or in the exploded modules directory.

That is, is the module present on disk. On the other hand, _jvmci_module_added is a test of whether an --add-modules option has been seen whose value included jdk.internal.vm.ci.

dougxc · 2025-05-16T07:01:29Z

That should be in PR and RFE (JBS) Descriptions! This was my main question about filed REF.

I've updated both the PR and JBS issue descriptions. Let me know if either still need improvement.

AlanBateman · 2025-05-16T08:22:20Z

src/hotspot/share/runtime/arguments.cpp

+    if (ClassLoader::is_module_observable("jdk.internal.vm.ci") && !UseJVMCINativeLibrary && !_jvmci_module_added) {
+      jio_fprintf(defaultStream::error_stream(),
+        "'+EnableJVMCI' requires '--add-modules=jdk.internal.vm.ci' when UseJVMCINativeLibrary is false\n");
+      return false;


There's something a bit uncomfortable about an error message naming a JDK internal module to specify to --add-modules.

If I understand correctly, +EnableJVMCI and libgraal is all good, the set of modules in the training run is the same as the production run. However, in the no libgraal scenario, and a mismatch between training and production runs (is that right)? then AOT is disabled. Is it really terrible to disable the AOTClassLinking optimizations in that scenario?

the set of modules in the training run is the same as the production run

I don't think that's true. That is, I don't think +EnableJVMCI is used in the training run is it @iklam ?

Is it really terrible to disable the AOTClassLinking optimizations in that scenario?

Depends if you care as much about VM startup when using libgraal as when using C2. Is there a reason why only one of these should have AOTClassLinking startup benefits?

There's also the issue of all the AOTClassLinking tests having to be disabled/ignored/problem listed in the libgraal mach5 tiers. This is what initially motivated @iklam and I to come up with this solution.

Depends if you care as much about VM startup when using libgraal as when using C2. Is there a reason why only one of these should have AOTClassLinking startup benefits?

I should have been clearer, my question/comment was about the no-libgraal case, not the libgraal case. With the proposed change, I think you are looking to print an error. I'm wondering why it can't continue to add jdk.internal.vm.ci to the set of root modules.

Ok, that's a good suggestion. I'll explore further.

And I now understand your first comment about "+EnableJVMCI and libgraal is all good" is in the context of having applied this PR. In that context, the set of modules in the training run is indeed the same as the production run.

I've pushed a commit that implements your suggestion and reduces the size of the overall change nicely. More importantly, I think it's a better design - thanks!

I like this latest version!

I ran a recent build of Oracle JDK 25 that has libjvmcicompiler.so (not including your changes):

$ ./bin/java -XX:+UnlockExperimentalVMOptions -XX:+UseGraalJIT -XX:+PrintFlagsFinal --version | \ egrep '(EnableJVMCI)|(UseJVMCICompiler)|(UseJVMCINativeLibrary)' bool EnableJVMCI = true {JVMCI product} {default} bool EnableJVMCIProduct = true {JVMCI product} {command line} bool UseJVMCICompiler = true {JVMCI product} {default} bool UseJVMCINativeLibrary = true {JVMCI product} {default} $ ./bin/java -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI -XX:+PrintFlagsFinal --version | \ egrep '(EnableJVMCI)|(UseJVMCICompiler)|(UseJVMCINativeLibrary)' bool EnableJVMCI = true {JVMCI experimental} {command line} bool EnableJVMCIProduct = false {JVMCI experimental} {default} bool UseJVMCICompiler = false {JVMCI experimental} {default} bool UseJVMCINativeLibrary = true {JVMCI experimental} {default}

So If you specify only -XX:+EnableJVMCI in the command-line, UseJVMCINativeLibrary will be true. As a result, with your latest version, the jdk.internal.vm.ci module is not added.

If you have an app that wants to use the jdk.internal.vm.ci API, you must specify both -XX:+EnableJVMCI and --add-modules=jdk.internal.vm.ci. Is this intentional?

If you have an app that wants to use the jdk.internal.vm.ci API, you must specify both -XX:+EnableJVMCI and --add-modules=jdk.internal.vm.ci.

You should only have to specify --add-modules=jdk.internal.vm.ci and that now sets +EnableJVMCI. If you also want libgraal to be used as the JIT (instead of C2), then you need to add -XX:+UseGraalJIT.

For the Truffle on Oracle JDK, this means:

--add-modules=jdk.internal.vm.ci: Use C2 for JIT ("hosted") compilation and libgraal for Truffle ("guest") compilation

--add-modules=jdk.internal.vm.ci -XX:+UseGraalJIT: Use libgraal for both JIT and Truffle compilation

Ah I forgot the setting on EnableJVMCI to true. Thanks for the explanation.

iklam · 2025-05-16T13:48:45Z

src/hotspot/share/runtime/arguments.cpp

+    PropertyList_unique_add(&_system_properties, "jdk.internal.vm.ci.enabled", "true",
+        AddProperty, UnwriteableProperty, InternalProperty);


What's the purpose of the jdk.internal.vm.ci.enabled property? Should it be enabled only if the jdk.internal.vm.ci module is added?

It exists to check in various Java-level entry points that the JVMCI VM support has been enabled so a nicer error message can be thrown that provides a possible corrective action. However, since it's now impossible to load the JVMCI module without enabling the JVMCI VM support, this is no longer of any use. I'll remove it.

dougxc · 2025-05-16T16:45:41Z

While testing this out on Graal, I discovered an interesting corner case.

public class UseJVMCIModule {
    public static void main(String[] args) {
        jdk.vm.ci.runtime.JVMCI.getRuntime();
    }
}

If the JVMCI module is indirectly added to the root module set, it results in an error:

java --add-modules=jdk.graal.compiler --add-exports=jdk.internal.vm.ci/jdk.vm.ci.runtime=ALL-UNNAMED UseJVMCIModule.java
Exception in thread "main" java.lang.InternalError: JVMCI is not enabled
	at jdk.internal.vm.ci/jdk.vm.ci.runtime.JVMCI.initializeRuntime(Native Method)
	at jdk.internal.vm.ci/jdk.vm.ci.runtime.JVMCI.getRuntime(JVMCI.java:64)
	at UseJVMCIModule.main(UseJVMCIModule.java:3)

That is, if an app wants to use the JVMCI module, it needs to explicitly communicate this to the launcher. By the time the root module graph is being initialized in ModuleBootstrap, it's too late to set EnableJVMCI.

I improved the error message to make this clear:

Exception in thread "main" java.lang.InternalError: JVMCI is not enabled. Must specify '--add-modules=jdk.internal.vm.ci' or '-XX:+EnableJVMCI' to the java launcher.

tkrodriguez · 2025-05-16T19:17:42Z

src/hotspot/share/jvmci/jvmciRuntime.hpp

@@ -35,6 +35,8 @@
 #include "gc/g1/g1CardTable.hpp"
 #endif // INCLUDE_G1GC

+#define JAVA_NOT_ENABLED_ERROR_MESSAGE "JVMCI is not enabled. Must specify '--add-modules=jdk.internal.vm.ci' or '-XX:+EnableJVMCI' to the java launcher."


You meant JVMCI_NOT_ENABLED_ERROR_MESSAGE I assume?

You meant JVMCI_NOT_ENABLED_ERROR_MESSAGE I assume?

Ha! Nice catch ;-)

tkrodriguez · 2025-05-16T19:33:25Z

...ompiler/jvmci/jdk.vm.ci.hotspot.test/src/jdk/vm/ci/hotspot/test/TestHotSpotJVMCIRuntime.java

@@ -170,6 +170,7 @@ public void jniEnomemTest() throws Exception {
            ProcessBuilder pb = ProcessTools.createLimitedTestJavaProcessBuilder(
                "-XX:+UnlockExperimentalVMOptions",
                "-XX:+EnableJVMCI",
+                "--add-modules=jdk.internal.vm.ci",


I stared at this for a while to understand why passing this option was required. It's a bit confusing that explicitly passing -XX:+EnableJVMCI has different effects based on the value of UseJVMCINativeLibrary. I think that if EnableJVMCI is passed on the command line then it should add the module even if libgraal is in use. So something like:
if ((!UseJVMCINativeLibrary || FLAG_IS_CMDLINE(EnableJVMCI) && ClassLoader::is_module_observable

I was not aware FLAG_IS_CMDLINE can be used for altering the semantics of a flag but there seems to be at least one precedent for it with UseCompactObjectHeaders. This is quite nice as now nothing needs to change for Truffle users in terms of enabling the Truffle optimized runtime (cc @chumer).

tkrodriguez

new version seems nice and clean.

dougxc · 2025-05-17T09:38:38Z

Any further feedback or concerns @iklam or @AlanBateman ?

iklam

Latest version looks good to me.

dougxc · 2025-05-18T19:17:28Z

In addition to loading the JVMCI module when -XX:+EnableJVMCI is on the command line, it should also be done when -XX:+EnableJVMCI is set by the jimage. The latter is how GraalVM sets some defaults and +EnableJVMCI is such a default. This ensures that the root module set is the same in training and production runs for AOTClassLinking on GraalVM.

AlanBateman · 2025-05-19T07:19:01Z

src/hotspot/share/runtime/arguments.cpp

+            _jvmci_module_added = true;
+          }
+        }
+      }


This only works if jdk.internal.vm.ci is specified to --add-modules, it won't set _jvmci_module_added if jdk.internal.vm.ci is resolved because some other module require it. As the module is JDK internal and doesn't export an API then I assume it would be rare-to-never to require it, is that right?

Correct. I realized this might be a bit confusing so improved the error message a little: #25240 (comment)
But as you say, it should never be encountered in practice.

AlanBateman · 2025-05-19T07:23:54Z

src/hotspot/share/jvmci/jvmciRuntime.hpp

@@ -35,6 +35,8 @@
 #include "gc/g1/g1CardTable.hpp"
 #endif // INCLUDE_G1GC

+#define JVMCI_NOT_ENABLED_ERROR_MESSAGE "JVMCI is not enabled. Must specify '--add-modules=jdk.internal.vm.ci' or '-XX:+EnableJVMCI' to the java launcher."


It's the exception message for an InternalError so maybe this is okay but in general I don't think you want to be asking users to have to name a JDK internal module.

I think anyone using a non-standard configuration for Graal would not be surprised about JVMCI and would actually appreciate the extra hint when they get things "wrong".

Assuming we want users to use -XX:+EnableJVMCI over --add-modules=jdk.internal.vm.ci for said reason, maybe flip the order?

vnkozlov

Looks good.

dougxc · 2025-05-21T19:20:47Z

Thanks for the reviews.

/integrate

openjdk · 2025-05-21T19:21:23Z

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

400c935: 8356904: Skip jdk/test/lib/process/TestNativeProcessBuilder on static-jdk
2dfbf41: 8351907: [XWayland] [OL10] Robot.mousePress() is delivered to wrong place
f1eead6: 8357452: Remove code span highlight in JavaDoc default stylesheet
... and 226 more: https://git.openjdk.org/jdk/compare/52a5583d691388f833c3aeb56ce92cbfb5d61274...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-21T19:21:33Z

@dougxc Pushed as commit 8153683.

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

--add-modules=jdk.internal.vm.ci implies -XX:+EnableJVMCI

171703a

dougxc mentioned this pull request May 14, 2025

8345826: [DO NOT MERGE] Do not automatically resolve jdk.internal.vm.ci when libgraal is used #23408

Closed

3 tasks

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels May 14, 2025

added comment in check_vm_args_consistency

0e8773e

dougxc marked this pull request as ready for review May 15, 2025 21:49

openjdk bot added the rfr Pull request is ready for review label May 15, 2025

vnkozlov reviewed May 15, 2025

View reviewed changes

graalvmbot mentioned this pull request May 16, 2025

[JDK-8345826] Replace +EnableJVMCI with --add-modules=jdk.internal.vm.ci. oracle/graal#11212

Open

AlanBateman reviewed May 16, 2025

View reviewed changes

resolve jdk.internal.vm.ci if +EnableJVMCI and -UseJVMCINativeLibrary

3436033

iklam reviewed May 16, 2025

View reviewed changes

dougxc added 3 commits May 16, 2025 16:24

fix TestHotSpotJVMCIRuntime

cc49ed0

removed use of jdk.internal.vm.ci.enabled property

7b30238

fixed comment

3cdef58

iklam approved these changes May 16, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 16, 2025

improved error message

1fe56b4

openjdk bot removed the ready Pull request is ready to be integrated label May 16, 2025

tkrodriguez reviewed May 16, 2025

View reviewed changes

dougxc added 2 commits May 16, 2025 22:45

JAVA_NOT_ENABLED_ERROR_MESSAGE -> JVMCI_NOT_ENABLED_ERROR_MESSAGE

cd615ef

load the JVMCI module if +EnableJVMCI is set on the command line

d9223af

tkrodriguez approved these changes May 16, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 16, 2025

iklam approved these changes May 18, 2025

View reviewed changes

load the JVMCI module if +EnableJVMCI is set in the jimage

196425f

openjdk bot removed the ready Pull request is ready to be integrated label May 18, 2025

AlanBateman reviewed May 19, 2025

View reviewed changes

swapped order of recommended options in error message

b74077f

tkrodriguez approved these changes May 19, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 19, 2025

graalvmbot mentioned this pull request May 21, 2025

[JDK-8345826] Adapt JDK-8345826: Do not automatically resolve jdk.internal.vm.ci when libgraal is used oracle/graal#11239

Open

iklam approved these changes May 21, 2025

View reviewed changes

vnkozlov approved these changes May 21, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 21, 2025

openjdk bot closed this May 21, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 21, 2025

		PropertyList_unique_add(&_system_properties, "jdk.internal.vm.ci.enabled", "true",
		AddProperty, UnwriteableProperty, InternalProperty);

8345826: Do not automatically resolve jdk.internal.vm.ci when libgraal is used #25240

8345826: Do not automatically resolve jdk.internal.vm.ci when libgraal is used #25240

Uh oh!

Conversation

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

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented May 14, 2025

Uh oh!

openjdk bot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented May 14, 2025

Uh oh!

tkrodriguez commented May 15, 2025

Uh oh!

dougxc commented May 15, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

vnkozlov commented May 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougxc commented May 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougxc commented May 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkrodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

dougxc commented May 17, 2025

Uh oh!

iklam left a comment

Choose a reason for hiding this comment

Uh oh!

dougxc commented May 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougxc commented May 14, 2025 •

edited by openjdk bot

Loading

openjdk bot commented May 14, 2025 •

edited

Loading

mlbridge bot commented May 15, 2025 •

edited

Loading

dougxc May 19, 2025 •

edited

Loading