Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8318027: Support alternative name to jdk.internal.vm.compiler #16189

Closed
wants to merge 5 commits into from

Conversation

dougxc
Copy link
Member

@dougxc dougxc commented Oct 13, 2023

The Graal code base has renamed its module to jdk.compiler.graal as part of preparations for Project Galahad. Due to the way Java modules work, this requires a JDK change. The core of the issue is that the service by which HotSpot requests a Graal compilation is defined in JVMCI. Since JVMCI is in the boot layer, the service can only be implemented by a provider in the boot layer and the package defining the service must be exported to the provider's defining module. This export currently targets jdk.internal.vm.compiler and so the binding fails for the new Graal module. To address this, this PR reflects the Graal module renaming, including adjusting the qualified export.

Edit: The target of the rename is now jdk.graal.compiler - see #16189 (comment)


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-8318027: Support alternative name to jdk.internal.vm.compiler (Enhancement - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16189

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 13, 2023

👋 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
Copy link

openjdk bot commented Oct 13, 2023

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

  • build
  • core-libs
  • graal
  • hotspot-compiler

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.

@openjdk openjdk bot added graal graal-dev@openjdk.org build build-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Oct 13, 2023
@dougxc dougxc changed the title 8318027: rename jdk.internal.vm.compiler* to jdk.compiler.graal* 8318027: support alternative name to jdk.internal.vm.compiler Oct 14, 2023
@dougxc dougxc changed the title 8318027: support alternative name to jdk.internal.vm.compiler 8318027: Support alternative name to jdk.internal.vm.compiler Oct 20, 2023
@dougxc dougxc marked this pull request as ready for review October 20, 2023 13:28
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 20, 2023
@mlbridge
Copy link

mlbridge bot commented Oct 20, 2023

Webrevs

@openjdk
Copy link

openjdk bot commented Oct 20, 2023

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

8318027: Support alternative name to jdk.internal.vm.compiler

Reviewed-by: erikj, ihse, kvn, alanb, mli

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

  • 99de9bb: 8317807: JAVA_FLAGS removed from jtreg running in JDK-8317039
  • 704c6ea: 8303525: Refactor/cleanup open/test/jdk/javax/rmi/ssl/SSLSocketParametersTest.java
  • 7c0a828: 8318649: G1: Remove unimplemented HeapRegionRemSet::add_code_root_locked
  • ff5c5b6: 8318643: +UseTransparentHugePages must enable +UseLargePages
  • fc29a2e: 8318082: ConcurrentModificationException from IndexWriter
  • 729f4c5: 8318507: G1: Improve remset clearing for humongous candidates
  • 4eab39d: 8318585: Rename CodeCache::UnloadingScope to UnlinkingScope
  • ffadd63: 8317868: Add @sealedGraph to MethodHandleDesc and descendants
  • ecd25e7: 8318484: Initial version of cdsConfig.hpp
  • a876beb: 8316741: BasicStroke.createStrokedShape miter-limits failing on small shapes
  • ... and 225 more: https://git.openjdk.org/jdk/compare/3105538de5569845547b40f243a994a95a84b48f...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 openjdk bot added the ready Pull request is ready to be integrated label Oct 20, 2023
Copy link
Member

@magicus magicus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build changes look trivially good.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you replaced pair of copyright years with one year in module-info.Java files? Instead of updating last year only.
Why also update 'since' there?
Even if you changed location these files existed already.

@dougxc
Copy link
Member Author

dougxc commented Oct 20, 2023

Why you replaced pair of copyright years with one year in module-info.Java files? Instead of updating last year only. Why also update 'since' there? Even if you changed location these files existed already.

The files may be renamed from git's point of view, but I would argue that these are new modules and so should have a new @since tag. Otherwise, someone seeing that jdk.compiler.graal existed @since 9 may be a little surprised/confused if they look for it in a JDK 9 binary. Same thinking applies to the copyright dates. Happy to fix it if this is the wrong way to think about it.

@vnkozlov
Copy link
Contributor

Copyright is associated with file and should be kept, only update last year.
I don't know rules about 'since'. But there will be no confusion for developers. Everyone knows that these files were created for Graal from the start.

@mbreinhold
Copy link
Member

The @since tag should reflect the release in which the tagged element was introduced (i.e., 22), not the release in which some predecessor of the element was introduced.

@dougxc
Copy link
Member Author

dougxc commented Oct 20, 2023

Ok, thanks for the clarification. What about the copyright dates on the module-info.java files?

@mbreinhold
Copy link
Member

Vladimir was right about the copyright dates — they should record the year in which the content was created and last updated, regardless of the content’s name.

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module config and test update looks fine.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good.

@jonathan-gibbons
Copy link
Contributor

jonathan-gibbons commented Oct 20, 2023

The proposed name jdk.compiler.graal makes it seem like the module is strongly related to the jdk.compiler module, which contains javac and related tools, whereas these modules are completely unrelated.

Is there not a better name that can be used (that does not begin with jdk.compiler)?

@jonathan-gibbons
Copy link
Contributor

/label compiler

@openjdk openjdk bot added the compiler compiler-dev@openjdk.org label Oct 20, 2023
@openjdk
Copy link

openjdk bot commented Oct 20, 2023

@jonathan-gibbons
The compiler label was successfully added.

@mbreinhold
Copy link
Member

Jon makes a good point. If memory serves, wasn’t the name jdk.graal.compiler suggested previously? That would avoid the overlap with the name of the existing jdk.compiler module.

@dougxc
Copy link
Member Author

dougxc commented Oct 21, 2023

Based on external discussion, agreement has been reached to rename to jdk.graal.compiler instead of jdk.compiler.graal.
Not only does this avoid the confusion with jdk.compiler pointed out by @jonathan-gibbons but it also establishes the namespace jdk.graal for Graal subprojects.

@dougxc
Copy link
Member Author

dougxc commented Oct 23, 2023

Thanks for all the reviews and careful consideration on this naming change. It will have long term consequences so getting it right is important.

/integrate

@openjdk
Copy link

openjdk bot commented Oct 23, 2023

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

  • c2efd77: 8295795: hsdis does not build with binutils 2.39+
  • 99de9bb: 8317807: JAVA_FLAGS removed from jtreg running in JDK-8317039
  • 704c6ea: 8303525: Refactor/cleanup open/test/jdk/javax/rmi/ssl/SSLSocketParametersTest.java
  • 7c0a828: 8318649: G1: Remove unimplemented HeapRegionRemSet::add_code_root_locked
  • ff5c5b6: 8318643: +UseTransparentHugePages must enable +UseLargePages
  • fc29a2e: 8318082: ConcurrentModificationException from IndexWriter
  • 729f4c5: 8318507: G1: Improve remset clearing for humongous candidates
  • 4eab39d: 8318585: Rename CodeCache::UnloadingScope to UnlinkingScope
  • ffadd63: 8317868: Add @sealedGraph to MethodHandleDesc and descendants
  • ecd25e7: 8318484: Initial version of cdsConfig.hpp
  • ... and 226 more: https://git.openjdk.org/jdk/compare/3105538de5569845547b40f243a994a95a84b48f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 23, 2023
@openjdk openjdk bot closed this Oct 23, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 23, 2023
@openjdk
Copy link

openjdk bot commented Oct 23, 2023

@dougxc Pushed as commit bd22d23.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org compiler compiler-dev@openjdk.org core-libs core-libs-dev@openjdk.org graal graal-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
8 participants