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

toddjonker · 2024-09-25T19:49:28Z

This disambiguates the situation where libgraal is "enabled" for use by non-CompilerBroker compilations, without being used as the JIT compiler.

Per discussion at #21120 (comment)

Grep shows that the property and related method are not used in this codebase. Tier1 tests on linux-x86_64-server-release pass cleanly.

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-8340974: Ambiguous name of jtreg property vm.libgraal.enabled (Enhancement - P4)

Reviewers

Doug Simon (@dougxc - Reviewer)
Paul Hohensee (@phohensee - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21190

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

Using diff file

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

Webrev

Link to Webrev Comment

This disambiguates the situation where libgraal is "enabled" for use by non-CompilerBroker compilations, without being used as the JIT compiler.

bridgekeeper · 2024-09-25T19:50:28Z

👋 Welcome back toddjonker! 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-25T19:51:43Z

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

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

Reviewed-by: dnsimon, phh

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

5d062e2: 8340576: Some JVMCI flags are inconsistent
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
... and 29 more: https://git.openjdk.org/jdk/compare/0b8c9f6d2397dcb480dc5ae109607d86f2b15619...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dougxc, @phohensee) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-09-25T19:52:14Z

@toddjonker The following label will be automatically applied to this pull request:

hotspot

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

toddjonker · 2024-09-25T20:46:07Z

/label graal

openjdk · 2024-09-25T20:47:59Z

@toddjonker
The graal label was successfully added.

mlbridge · 2024-09-25T21:41:04Z

Webrevs

01: Full - Incremental (59a8ed3c)
00: Full (386150d4)

dougxc · 2024-09-26T07:23:35Z

test/jtreg-ext/requires/VMProps.java

     * @return true if libgraal is used as JIT compiler.
     */
-    protected String isLibgraalEnabled() {
+    protected String isLibgraalJit() {


I slightly prefer isLibgraalJIT as this acronym is (most) capitalized in the code base.

You should also rename isLibgraalEnabled to isLibgraalJIT in test/lib/jdk/test/whitebox/code/Compiler.java for consistency.

Renamed both. I was being conservative by limiting scope of change to the relevant interface, and felt that the existing name was reasonable in the Compiler scope.

dougxc

LGTM.

toddjonker · 2024-09-26T20:44:06Z

/integrate auto

openjdk · 2024-09-26T20:45:05Z

@toddjonker This pull request will be automatically integrated when it is ready

openjdk · 2024-09-26T20:45:25Z

/integrate

openjdk · 2024-09-26T20:45:39Z

@openjdk[bot]
Your change (at version 59a8ed3) is now ready to be sponsored by a Committer.

benty-amzn · 2024-09-26T21:28:06Z

/sponsor

openjdk · 2024-09-26T21:29:31Z

@benty-amzn Only Committers are allowed to sponsor changes.

phohensee · 2024-09-26T21:36:48Z

/sponsor

openjdk · 2024-09-26T21:38:09Z

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

5d062e2: 8340576: Some JVMCI flags are inconsistent
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
... and 29 more: https://git.openjdk.org/jdk/compare/0b8c9f6d2397dcb480dc5ae109607d86f2b15619...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-26T21:38:18Z

@phohensee @toddjonker Pushed as commit 2349bb7.

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

Rename jtreg property vm.libgraal.enabled to vm.libgraal.jit.

386150d

This disambiguates the situation where libgraal is "enabled" for use by non-CompilerBroker compilations, without being used as the JIT compiler.

openjdk bot added the hotspot hotspot-dev@openjdk.org label Sep 25, 2024

openjdk bot added the graal graal-dev@openjdk.org label Sep 25, 2024

toddjonker changed the title ~~Rename jtreg property vm.libgraal.enabled to vm.libgraal.jit.~~ 8340974: Rename jtreg property vm.libgraal.enabled to vm.libgraal.jit. Sep 25, 2024

toddjonker changed the title ~~8340974: Rename jtreg property vm.libgraal.enabled to vm.libgraal.jit.~~ 8340974: Ambiguous name of jtreg property vm.libgraal.enabled Sep 25, 2024

toddjonker marked this pull request as ready for review September 25, 2024 21:35

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

toddjonker mentioned this pull request Sep 25, 2024

8340576: Some JVMCI flags are inconsistent #21120

Closed

3 tasks

dougxc reviewed Sep 26, 2024

View reviewed changes

Rename Compiler.isLibgraalEnabled to isLibgraalJIT

59a8ed3

dougxc 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 auto Expect automatic integration when ready; no further action required label Sep 26, 2024

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

phohensee approved these changes Sep 26, 2024

View reviewed changes

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 sponsor Pull request is ready to be sponsored labels Sep 26, 2024

toddjonker deleted the vm-libgraal-jit branch September 26, 2024 21:56

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

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

Uh oh!

Conversation

toddjonker commented Sep 25, 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 25, 2024

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Sep 25, 2024

Uh oh!

toddjonker commented Sep 25, 2024

Uh oh!

openjdk bot commented Sep 25, 2024

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

dougxc Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

toddjonker Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

dougxc left a comment

Choose a reason for hiding this comment

Uh oh!

toddjonker commented Sep 26, 2024

Uh oh!

openjdk bot commented Sep 26, 2024

Uh oh!

openjdk bot commented Sep 26, 2024

Uh oh!

openjdk bot commented Sep 26, 2024

Uh oh!

benty-amzn commented Sep 26, 2024

Uh oh!

openjdk bot commented Sep 26, 2024

Uh oh!

phohensee 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!

4 participants

toddjonker commented Sep 25, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 25, 2024 •

edited

Loading

mlbridge bot commented Sep 25, 2024 •

edited

Loading