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

8345169: Implement JEP 503: Remove the 32-bit x86 Port #23906

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shipilev
Copy link
Member

@shipilev shipilev commented Mar 4, 2025

This PR implements JEP 503: Remove the 32-bit x86 Port.

The JEP is proposed to target 25, we would not integrate until JEP is ready. Reviews are appreciated meanwhile.

This is only the removal of obvious 32-bit x86 parts, mostly files with x86_32 in their name. Those are only built when build system knows we are compiling for x86_32. There is therefore no impact on x86_64. The approach for removing x86_32 files only also makes this PR borderline trivial, and requires no additional testing beyond normal pre-integration checks.

The rest of the code is quite heavily intertwined with x86_64 and/or Zero, and would require accurate untangling. It would be much easier to review and test once we purge the free-standing parts of 32-bit x86 port, which is also a bulk of the port. The tangling with 32-bit x86 Zero is also why I did not touch most of the build system paths that handle x86. There is JDK-8351148 umbrella that tracks further cleanup work. One can peek the final state that can be reached with all the cleanups in my earlier exploratory #22567.

Additional testing:

  • Linux x86_32 Server fastdebug, make bootcycle-images (now fails configure)
  • Linux x86_64 Server fastdebug, make bootcycle-images (still works)
  • Linux x86_32 Zero fastdebug, make bootcycle-images (still works)
  • Linux x86_64 Zero fastdebug, make bootcycle-images (still works)

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
  • Change requires a JEP request to be targeted

Issues

  • JDK-8345169: Implement JEP 503: Remove the 32-bit x86 Port (Sub-task - P4)
  • JDK-8345168: JEP 503: Remove the 32-bit x86 Port (JEP)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23906

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

Using diff file

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

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 4, 2025

👋 Welcome back shade! 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.

@shipilev shipilev force-pushed the JDK-8345169-32bit-x86-be-gone branch from bcee898 to 1ff9bbc Compare March 4, 2025 16:53
@openjdk
Copy link

openjdk bot commented Mar 4, 2025

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk
Copy link

openjdk bot commented Mar 4, 2025

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

  • build
  • hotspot
  • shenandoah

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.

@shipilev
Copy link
Member Author

shipilev commented Mar 4, 2025

/jep JEP-503

@openjdk
Copy link

openjdk bot commented Mar 4, 2025

@shipilev
This pull request will not be integrated until the JEP-503 has been targeted.

@openjdk openjdk bot added the jep label Mar 4, 2025
@shipilev shipilev marked this pull request as ready for review March 5, 2025 16:50
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 5, 2025
@mlbridge
Copy link

mlbridge bot commented Mar 5, 2025

Webrevs

Copy link
Contributor

@iwanowww iwanowww left a comment

Choose a reason for hiding this comment

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

Hotspot changes look good to me.

I fully support removing x86-32-specific files first and then clean up x86-32-specific code in x86-specific and shared files (e.g., guarded by #ifndef _LP64).

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.

So it will be stacked PRs which you will combine for final integration?

@vnkozlov
Copy link
Contributor

vnkozlov commented Mar 5, 2025

This is confusing. This PR is part of changes so it can't be "Implement JEP 503: Remove the 32-bit x86 Port" and should be subtask of Umbrella RFE.
Am I missing something?

@iwanowww
Copy link
Contributor

iwanowww commented Mar 5, 2025

There's a wide variety of options to justify the goal of the JEP. A bare minimum would be to just remove x86-32 build support. And on the other side of the spectrum the current patch would be accompanied by all x86-32-specific code and all the features used exclusively by x86-32 port.

During previous round of discussions I expressed my preference as keeping JEP implementation simple and perform all non-trivial cleanups as follow-up RFEs. IMO it enables swift removal (and eliminates the burden to keep x86-32 port alive during ongoing development work) while keeping incremental cleanup activities at comfortable pace.

Proposed patch perfectly justifies my preference.

@vnkozlov
Copy link
Contributor

vnkozlov commented Mar 5, 2025

To clarify. I am completely agree with changes in this PR - I approved it.

My concern is the Title of this PR and JBS entry. So I want to understand the steps we do with this PR and following changes covered by numbers of subtask pointed by Aleksey.

So what, @iwanowww, you say is that this PR is indeed implementation of the JEP.
And all subtasks listed in Umbrella RFE are following up RFEs after we integrated the JEP.

Do I understand that correctly?

Why not do what Ioi did for AOT class loading JEP? I mean, to have depending PRs which are combined into one implementation push.

@iwanowww
Copy link
Contributor

iwanowww commented Mar 6, 2025

So what, @iwanowww, you say is that this PR is indeed implementation of the JEP.
And all subtasks listed in Umbrella RFE are following up RFEs after we integrated the JEP.
Do I understand that correctly?

Yes.

Why not do what Ioi did for AOT class loading JEP? I mean, to have depending PRs which are combined into one implementation push.

It's definitely an option. But, most likely, there'll be some overlooked cases anyway (leading to additional followup RFEs). And the more convoluted the changes are the harder it is to validate their correctness, thus increasing the risks for product stability and delaying the integration. (I'm not sure how much time Aleksey and other contributors want to volunteer to this project.)

Also, in case of AOT JEP the situation was quite the opposite: it started with a huge patch which was split into multiple mostly independent parts to streamline its review. For x86-32 code removal there's no such patch prepared yet and the complete scope of work is not clear yet.

IMO the crucial part is to get the port officially retired. After that the rest can become a good source of starter tasks :-)

@vnkozlov
Copy link
Contributor

vnkozlov commented Mar 6, 2025

Okay. Thank you for explaining.

@dholmes-ora
Copy link
Member

I am also a bit puzzled by the JEP/JBS strategy here. I would expect a bunch of dependent PRs that then get integrated together as "The Implementation of JEP 503". I understand things may be missed that require some follow up RFE's but I don't think we should start from that position and have a large chunk of work not be done under the JEP umbrella.

@shipilev
Copy link
Member Author

shipilev commented Mar 6, 2025

Basically what @iwanowww said: this PR is the removal of x86_32 port.

After this PR integrates, it is not possible to build x86_32, because the core implementation of it is missing, and build system would refuse to even try building it. So this removes x86_32 port as the feature, atomically, matching the title and intent of the JEP. Then, follow-up subtasks RFE would clean up the parts of Hotspot that were added to support various x86_32-specific features, and are no longer needed anymore.

Honestly, I also believed the complete PR that cleans up every dusty corner at once would be more straight-forward. But then I tried it at #22567. After investing a few full days on that draft PR, and listening to what people said about it, I firmly changed my mind, and can conclude that singular PR or series of stacked PRs are not a great way to go with this removal.

The massive drawbacks of complete/stacked PR are now obvious to me:

  1. It is hard to review. The complete PR is huge, 210+ files affected. A lot of removals are logically connected across different files, and while they are simple in isolation, it is hard for a reviewer to separate several cleanups in large PRs. Stacked PRs would help some, but:
  2. It accrues merge conflicts very fast. This happens even when mainline is somewhat idle without large feature integrations. I did complete PR near New Year holidays, and it was already a headache. I expect this work to be even harder once we are closer to RDP1. It would be even more tedious with a chain of 10+ stacked PRs, as I got the preview of this when rebasing the stack of atomic commits in the complete draft PR several times.
  3. It is hard to reach consensus on. Non-trivial changes require thorough review, and cobbling together multiple non-trivial changes require polynomially more coordination with reviewers for different subsystems. I have seen this in Win32 port removal, so for a large PR like that I expect multiple, weeks-long review and amendment sessions. Which conspires with (1) and (2).
  4. It is easy to introduce/overlook bugs. I already did this once in a complete PR when I accidentally removed the wrong part of C1 regalloc code, and it started ever so slightly misbehaving. And it was not obvious, because it was obscured by other changes in the vicinity, and it only failed one test in tier4. This conspires with (1), (2) and (3).
  5. It would introduce a single changeset that would be hard to bisect when things go wrong. And the things would go wrong, because of (1), (4) and partially by new opportunities presented by (2). For the C1 bug I mentioned above, I was able to quickly nail it through the bisection of my stack of atomic commits. That stack would not be available once we squash the commits/PRs before the integration.

So while on a surface it might look more enticing to purge everything at once, the amount of hassle we would endure is hard to justify. Doing this PR for port removal + multiple post-removal cleanups piecewise lets us reach the same final state without extra work, while doing so at leisurely pace and maintaining more convenient code history for future bug hunts.

Bottom-line: Let's not make our own lives harder unnecessarily. Atomic commits FTW.

Copy link
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

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

I agree with @iwanowww's and @shipilev comments. I would like to see this be the JEP implementation and the additional cleanups, particularly in the interpreter, handled one by one. I don't see any advantage for one big integration push. It'll be disruptive and for this, there is no scenario where this would be helpful to any future work.
When Aleksey sent out the original PR there were cleanups that needed explanation. Finding the explanations in the big PR is a pain for scrolling. And the reviewers for that part of the change were a different set than ones needed for this change. Again for no benefit.

@coleenp
Copy link
Contributor

coleenp commented Mar 6, 2025

Also @shipilev I'm jealous of all your code removal. :) Well done getting agreement on this change.

Use --enable-deprecated-ports=yes to suppress this error.]))
fi
fi
# There are no deprecated ports. This option is left to be consistent with future deprecations.
Copy link
Member

Choose a reason for hiding this comment

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

Please remove. Old code is always present in git history if you want to reuse it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't mind removing it, my concern would be to remember this option was there! I guess it is okay to re-re-invent it later, possibly under a different name, when the next port gets deprecated.

Copy link
Member

Choose a reason for hiding this comment

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

It's no that important, no. I'm not sure if previous deprecated ports were handles exactly like this.

And you can always do like git log | grep -i "remove .* port" to find the change it was removed in, and look what it did...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants