8340576: Some JVMCI flags are inconsistent #21120

dougxc · 2024-09-22T11:51:20Z

This PR replaces some uses of UseJVMCICompiler with EnableJVMCI so that JVMCI code paths are taken when JVMCI is only used for non-CompilerBroker compilations.

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-8340576: Some JVMCI flags are inconsistent (Bug - P5)

Reviewers

@tzezula (no known openjdk.org user name / role) Review applies to 470f9a7c
Tom Rodriguez (@tkrodriguez - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21120

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-22T11:52:14Z

👋 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 · 2024-09-22T11:53:51Z

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

8340576: Some JVMCI flags are inconsistent

Reviewed-by: never

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 50 new commits pushed to the master branch:

1447967: 8339261: Logs truncated in test javax/net/ssl/DTLS/DTLSRehandshakeTest.java
a02d895: 8333403: Write a test to check various components events are triggered properly
bb040ef: 8340983: Use index and definition tags in Object and Double
8225a5f: 8340981: Update citations to "Hacker's Delight"
aceae76: 8339460: CDS error when module is located in a directory with space in the name
aeaa4f7: 8336895: BufferedReader doesn't read full \r\n line ending when it doesn't fit in buffer
376056c: 8336468: Reflection and MethodHandles should use more precise initializer checks
e36ce5f: 8336942: Improve test coverage for class loading elements with annotations of different retentions
95d3e9d: 8339560: Unaddressed comments during code review of JDK-8337664
47fcf5a: 8340687: Open source closed frame tests Azure Pipelines continuous integration (without running tests, for now) #1
... and 40 more: https://git.openjdk.org/jdk/compare/e1c4d3039f6b5106ce3f65d50f607eacc2a8d168...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 · 2024-09-22T11:54: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.

mur47x111 · 2024-09-22T14:28:46Z

src/hotspot/share/jvmci/jvmci_globals.cpp

  CHECK_NOT_SET(JVMCIHostThreads,             UseJVMCICompiler)
  CHECK_NOT_SET(LibJVMCICompilerThreadHidden, UseJVMCICompiler)

+  if ((UseJVMCICompiler || EnableJVMCI) &&


Doesn't UseJVMCICompiler require EnableJVMCI?

This will conflict with my change b755046.

No problem - I'll resolve it once your PR is merged.

Based just off the flag docs, I think its easy to think that +EnableJVMCI is a prerequisite to enabling the other JVMCI flags, along the line of +UnlockExperimentalVMOptions. (That was for sure my newbie reading.)

Perhaps its docs (here) should be updated to say "Defaults to true if UseJVMCICompiler is true"?

TBH I can't wrap my head around what +EnableJVMCI means; these options have a few chains of "this enables that" making it hard to grok the interactions.

I've pushed e26e68d to try clarify this a bit.

I know it's a little confusing and apologize. When JVMCI is no longer experimental, this should become much clearer.

tzezula · 2024-09-23T07:42:12Z

Looks good to me.

src/hotspot/share/compiler/oopMap.inline.hpp

…njunctive tests of both

mlbridge · 2024-09-24T11:55:44Z

Webrevs

01: Full - Incremental (e26e68d9)
00: Full (fcc3ece0)

toddjonker · 2024-09-24T22:00:02Z

Studying these recent changes led me back to #14851 which added jtreg propeties:

jdk.hasLibgraal: the libgraal shared library file is present
vm.libgraal.enabled: libgraal is used as JIT compiler

The latter now feels misleading, since libgraal can be "enabled" for use by non-CompilerBroker compilations, without being used as the JIT compiler. (I'm here b/c we're assembling a distro doing exactly that.)

Would it make sense to rename the latter, to reduce ambiguity in the tests?

dougxc · 2024-09-24T22:09:42Z

Would it make sense to rename the latter, to reduce ambiguity in the tests?

Sounds reasonable to me. Maybe vm.libgraal.jit? The good news is that there are no current tests using this predicate as far as I can see.

Want to take the lead on this?

toddjonker · 2024-09-24T22:22:31Z

Would it make sense to rename the latter, to reduce ambiguity in the tests?

Sounds reasonable to me. Maybe vm.libgraal.jit? The good news is that there are no current tests using this predicate as far as I can see.

Want to take the lead on this?

I like that alternative, and yes I'll work up the patch. I find myself need to fix several jtreg tests to handle this kind of configuration, so it's relevant to my goals.

toddjonker · 2024-09-25T21:38:49Z

@dougxc I cut an issue https://bugs.openjdk.org/projects/JDK/issues/JDK-8340974 and posted a PR #21190

This is my first JDK issue and fix; apologies if I'm getting the process wrong. I wasn't sure if I should tag you on either (or how).

tkrodriguez

This is a nice cleanup.

dougxc · 2024-09-26T19:35:08Z

Thanks for the reviews.

/integrate

openjdk · 2024-09-26T19:36:30Z

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

1447967: 8339261: Logs truncated in test javax/net/ssl/DTLS/DTLSRehandshakeTest.java
a02d895: 8333403: Write a test to check various components events are triggered properly
bb040ef: 8340983: Use index and definition tags in Object and Double
8225a5f: 8340981: Update citations to "Hacker's Delight"
aceae76: 8339460: CDS error when module is located in a directory with space in the name
aeaa4f7: 8336895: BufferedReader doesn't read full \r\n line ending when it doesn't fit in buffer
376056c: 8336468: Reflection and MethodHandles should use more precise initializer checks
e36ce5f: 8336942: Improve test coverage for class loading elements with annotations of different retentions
95d3e9d: 8339560: Unaddressed comments during code review of JDK-8337664
47fcf5a: 8340687: Open source closed frame tests Azure Pipelines continuous integration (without running tests, for now) #1
... and 40 more: https://git.openjdk.org/jdk/compare/e1c4d3039f6b5106ce3f65d50f607eacc2a8d168...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-26T19:36:36Z

@dougxc Pushed as commit 5d062e2.

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

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Sep 22, 2024

dougxc force-pushed the JDK-8340576 branch from 5c8a096 to b0b2382 Compare September 22, 2024 12:31

mur47x111 reviewed Sep 22, 2024

View reviewed changes

dougxc changed the title ~~8340576: some JVMCI flags are inconsistent~~ 8340576: Some JVMCI flags are inconsistent Sep 23, 2024

disentangle EnableJVMCI and UseJVMCICompiler

470f9a7

dougxc force-pushed the JDK-8340576 branch from b0b2382 to 470f9a7 Compare September 23, 2024 06:45

tzezula approved these changes Sep 23, 2024

View reviewed changes

tkrodriguez reviewed Sep 23, 2024

View reviewed changes

src/hotspot/share/compiler/oopMap.inline.hpp Outdated Show resolved Hide resolved

dougxc added 3 commits September 24, 2024 08:57

since UseJVMCICompiler implies EnableJVMCI, remove the latter from co…

1ea08b1

…njunctive tests of both

fix incorrect code in oopMap.inline.hpp

afde3d4

Merge branch 'master' into JDK-8340576

fcc3ece

dougxc force-pushed the JDK-8340576 branch from cd1ce4d to fcc3ece Compare September 24, 2024 11:47

dougxc marked this pull request as ready for review September 24, 2024 11:51

openjdk bot added the rfr Pull request is ready for review label Sep 24, 2024

clarified doc for EnableJVMCI and UseJVMCINativeLibrary

e26e68d

toddjonker mentioned this pull request Sep 25, 2024

8340974: Ambiguous name of jtreg property vm.libgraal.enabled #21190

Closed

3 tasks

tkrodriguez approved these changes Sep 26, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 26, 2024

openjdk bot added the integrated Pull request has been integrated label Sep 26, 2024

openjdk bot closed this Sep 26, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 26, 2024

8340576: Some JVMCI flags are inconsistent #21120

8340576: Some JVMCI flags are inconsistent #21120

Uh oh!

Conversation

dougxc commented Sep 22, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Sep 22, 2024

Uh oh!

openjdk bot commented Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 22, 2024

Uh oh!

mur47x111 Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

dougxc Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

tzezula Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

dougxc Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

toddjonker Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougxc Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

tzezula commented Sep 23, 2024

Uh oh!

Uh oh!

mlbridge bot commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

toddjonker commented Sep 24, 2024

Uh oh!

dougxc commented Sep 24, 2024

Uh oh!

toddjonker commented Sep 24, 2024

Uh oh!

toddjonker commented Sep 25, 2024

Uh oh!

tkrodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

dougxc commented Sep 26, 2024

Uh oh!

openjdk bot commented Sep 26, 2024

Uh oh!

openjdk bot commented Sep 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

dougxc commented Sep 22, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 22, 2024 •

edited

Loading

toddjonker Sep 24, 2024 •

edited

Loading

mlbridge bot commented Sep 24, 2024 •

edited

Loading