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
8297864: Dead code elimination #11439
Conversation
|
/label remove serviceability |
@robehn |
12da0c8
to
4dd56f2
Compare
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, splitting this up would just waste time. Please run this tool more often! I see some things I should have removed in here, and some things that were proactively added can be added when there's code to use them.
Looks wonderful. Will make the code easier to maintain.
@robehn This change now passes all automated pre-integration checks. 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 36 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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I think this warrants closer inspection to ensure there isn't a bug because something is unused (e.g. is_in_asgct()
code in forte.cpp).
Also IIRC Loom knowingly integrated a lot of code that will only come in to play in future previews and so it may not be appropriate to cull it now. Perhaps @pchilano can comment?
Maybe this flag was read by solaris studio or so, way back. No readers today.
The code is still in loom/fibers, when they merge with mainline they can choice to keep it in loom/fiber. |
I took a look at the Loom related dead code. I see it's mostly just unused short helper methods, but other than that don't see any removal that would add complexity with future work. If some of those actually turn out to be useful we can bring them back with the related change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Thanks,
Patricio
FTR: The |
/integrate |
Going to push as commit d523d9d.
Your commit was automatically rebased without conflicts. |
Please consider removing all this dead code.
I did not have any good way of splitting this up and I did not see any benefit is just splitting it up 'just because'.
There is one commit per file couple.
Not much else to say, passes t1-4 and I can build:
linux-aarch64-debug,linux-arm32-debug,linux-ppc64le-debug,linux-s390x-debug,linux-x86,linux-x86-debug,macosx-aarch64-debug,macosx-x64,macosx-x64-debug,windows-x64-debug,windows-x86-debug and zero-x64-debug
Some of them I also built release.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11439/head:pull/11439
$ git checkout pull/11439
Update a local copy of the PR:
$ git checkout pull/11439
$ git pull https://git.openjdk.org/jdk pull/11439/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11439
View PR using the GUI difftool:
$ git pr show -t 11439
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11439.diff