8333566: Remove unused methods #19550
Conversation
|
👋 Welcome back cslucas! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
changed the title
|
@JohnTortugo this pull request can not be integrated into git checkout unused-methods
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 |
openjdk
bot
added
the
merge-conflict
|
@JohnTortugo 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. |
openjdk
bot
added
graal
openjdk
bot
removed
the
merge-conflict
JohnTortugo
force-pushed
the
unused-methods
branch
from
733d0a7 to
5b18baa
Compare
| static void set_has_CompareTrap() { _features[0] |= GnrlInstrExtFacilityMask; } | ||
| static void set_has_RelativeLoadStore() { _features[0] |= GnrlInstrExtFacilityMask; } | ||
| static void set_has_GnrlInstrExtensions() { _features[0] |= GnrlInstrExtFacilityMask; } | ||
| static void set_has_ProcessorAssist() { _features[0] |= ProcessorAssistMask; } |
There was a problem hiding this comment.
This looks incorrect; there exist a second definition below;
| // * No condition for this * void ALWAYSINLINE jcxz(Label& L, bool maybe_short = true) { jcc(Assembler::cxz, L, maybe_short); } | ||
| // * No condition for this * void ALWAYSINLINE jecxz(Label& L, bool maybe_short = true) { jcc(Assembler::cxz, L, maybe_short); } | ||
|
|
||
| // Short versions of the above |
There was a problem hiding this comment.
These all branch instructions were added recently #18893 for JDK-8320448 which is not pushed yet. So I will suggest to not remove them.
| // | ||
| // Feature identification which can be affected by VM settings | ||
| // | ||
| static bool supports_cpuid() { return _features != 0; } |
There was a problem hiding this comment.
I suggest to not touch this file. Some CPU features could used in a future.
There was a problem hiding this comment.
I feel very uncomfortable with this PR, at least as far as s390 is concerned. Many of the methods now set to be removed have been implemented with "implementation completeness" in mind. Not being used currently does not allow the implication of being obsolete. The approach on s390 has always been to provide support for potentially useful new instructions once they become available. Later exploitation can then focus on the use, without bloating the PR with low-level assembler* declarations.
There are a few exceptions, though. One is the code around _atomic_memory_operation_lock. That seems to be a leftover which simply was forgotten. Same seems true for pd_relocate_CodeBlob
|
Hi, Removing dead code is great, but it has to be done with reasoning behind it and preferably in smaller batches, so as to be reviewable. I also don't understand why you comment out tests. If you can make these into smaller and more localized PRs, then I'll be happy to take a look if I think I'm the right person to review it. All the best, |
|
Some parts of the code, such as the assemblers, can be seen as tools that we have in our shed so that we can write other powerful code. If you have a shed full of tools, then naturally you can go through the shed and get rid of the tools we don't seem to currently use. Who needs a spade anyway? Nobody has used that spade for a year! Except that eventually, the day always comes when you need a spade. Since you have now thrown away the only spade in the shed, you will find yourself with the option to either 1) try to make do with a trowel, which is horrible but might work as a hack. Or 2) you have to make a new spade yet again. And no, we can't buy a ready made spade. It can be very annoying when you have what would seemingly be a trivial patch, but then you find out you won the lottery and you are apparently the first person in a while that needed a testl with a memory operand comparing against a 32 bit immediate, and have to go and read ISA manuals to figure out how to encode this thing correctly. It adds a large amount of extra work to add support for something that we should be able to take for granted. I'm not a big fan of throwing away all the tools we have in the shed just because they haven't been used in a while. I don't want to dig my next hole with a trowel, nor do I want to build a new spade that we already have. |
|
This seems to me like a classic case of not reading the instructions. @JohnTortugo, please read the OpenJDK Developers' Guide before posting a PR with a huge change. There are sections in there that covers this exact case. The section Contributing to an OpenJDK Project really is mandatory reading before doing anything in OpenJDK. |
|
Closing this as not relevant. Thank you for the reviews. |
7 participants
Please, consider this patch to remove unused methods from the code base. To the best of my knowledge, these methods are only defined but never used.
Here is a list with names of delete methods: https://gist.github.com/JohnTortugo/fccc29781a1b584c03162aa4e160e874
Tested with Linux x86_64 tier1-4, GHA, and only cross building to other platforms.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19550/head:pull/19550$ git checkout pull/19550Update a local copy of the PR:
$ git checkout pull/19550$ git pull https://git.openjdk.org/jdk.git pull/19550/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19550View PR using the GUI difftool:
$ git pr show -t 19550Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19550.diff
Webrev
Link to Webrev Comment