8382582: Remove the experimental JVMCI feature#30834
Conversation
|
👋 Welcome back mhaessig! A progress list of the required criteria for merging this PR into |
|
/contributor add @TobiHartmann |
|
@mhaessig 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: 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 6 new commits pushed to the
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 |
|
@mhaessig |
|
@mhaessig |
|
@mhaessig The following labels will be automatically applied to this pull request:
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. |
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
liach
left a comment
There was a problem hiding this comment.
Core library changes look fine.
|
/label remove security |
|
@wangweij |
|
@mhaessig this pull request can not be integrated into git checkout remove-jvmci
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
Co-Authored-By: Vladimir Ivanov <vladimir.x.ivanov@oracle.com>
mhaessig
left a comment
There was a problem hiding this comment.
Thank you all for reviewing. I addressed all of your comments in the new commits and I merged master.
| @@ -1771,28 +1765,18 @@ class vector_VectorPayload : AllStatic { | |||
| }; | |||
|
|
|||
| class java_lang_Integer : AllStatic { | |||
There was a problem hiding this comment.
Removed in cf16786. I think those are the ones you meant?
liach
left a comment
There was a problem hiding this comment.
Core libs and javadoc changes look trivially fine.
There was a problem hiding this comment.
I don't know if we want to remove this file or do the jdk.jsobject-style removal in the symbols - it might be okay for us to nuke these internal modules this way.
There was a problem hiding this comment.
Hmm.
I think removing the files for JDK releases from "M" (i.e. 22) through "R" (i.e. 27) i needs some more consideration, Removing the files in the current release is much less problematic.
There was a problem hiding this comment.
Sorry, I somehow missed this. Normally, I would say the historical data (i.e. src/jdk.compiler/share/data/symbols) should not be touched when removing something. But, in this case, these files seem more like an accident than real history. They are not used (not used in symbols), and so are not part of ct.sym, and so do not affect --release. It is possible the script malfunctioned when producing the historical record for JDK 22, as there are a few more files like this for JDK 22 (jdk.internal.ed-M.sym.txt, jdk.internal.jvmstat-M.sym.txt, jdk.internal.le-M.sym.txt, jdk.internal.opt-M.sym.txt).
So, I am fine with removing JVMCI-related files. Separately, we might look at cleaning up the other stray JDK 22 files and/or investigating what happened.
coleenp
left a comment
There was a problem hiding this comment.
The classfile code looks good. Thanks for the extra cleanup.
|
/csr needed |
|
@jddarcy has indicated that a compatibility and specification (CSR) request is needed for this pull request. @mhaessig please create a CSR request for issue JDK-8382582 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
Note, JVMCI was always JDK internal experimental feature. I am not sure what the goal of CSR will be here. |
liach
left a comment
There was a problem hiding this comment.
Removal of the javac symbol files should be fine. javac's --add-modules already does not work with --release for these removed modules.
There was a problem hiding this comment.
nit: Copyright year not updated here (other files were bumped to 2026).
| * Copyright (c) 2007, 2026, Oracle and/or its affiliates. All rights reserved. |
This PR removes the experimental JVMCI feature and all usages and references of it, including references to Graal. For more details on the rationale, please see the issue description.
This change was constructed in several steps:
([Jj][Vv][Mm][Cc][Ii])|([Gg]raal)|vm\.ci|[Gg]alahadand immediately obvious dead code.git blamefor each file for JVMCI related commits and check if that code is now dead.Note to Reviewers
Each commit is limited to a subfolder or a change spanning further so you can take look at a piece of the codebase you are familiar with. I recommend you look at individual commits and select "Ignore Whitespace" in the Github UI.
Testing
Progress
Issues
Reviewers
Contributors
<thartmann@openjdk.org><mikael@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30834/head:pull/30834$ git checkout pull/30834Update a local copy of the PR:
$ git checkout pull/30834$ git pull https://git.openjdk.org/jdk.git pull/30834/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30834View PR using the GUI difftool:
$ git pr show -t 30834Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30834.diff
Using Webrev
Link to Webrev Comment