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

Enable LTO building on supported platforms by default #13192

Closed
wants to merge 1 commit into from

Conversation

tamara-schmitz
Copy link
Contributor

@tamara-schmitz tamara-schmitz commented Feb 7, 2023

While there was a PR before that made LTO builds possible, it was not enabled by default. Why not try flip the switch for release builds?

Link Time Optimization or Interprocedural Optimization is a build time optimisation gives the compiler information about how the individual source code files will be linked. This generally leads to a speed up for most C/C++ programs. This feature is enabled on a lot of major software projects as well as Linux distributions.
A good technical optimisation of how LTO works can be found here: https://www.llvm.org/docs/LinkTimeOptimization.html

Understandably it is still possible that one could run into issues here. If we do encounter them, then this PR can include fixes for broken code. If a build environment does not support LTO, the feature still remains off.

My proposal is to just merge this PR once it has been tested somewhat and if users do come back with issues, revert the PR and fix the issues.

To do

This PR is a Work in Progress.

  • Test building on the minetest CI
  • Ask others to run builds locally

How to test

Apply the change and build locally / in your buildsystem, or grab builds from the CI artifacts.

@wsor4035 wsor4035 added the @ Build CMake, build scripts, official builds, compiler and linker errors label Feb 7, 2023
@tamara-schmitz
Copy link
Contributor Author

Results so far testing the CI:
the gcc 5 and clang 3.9 builds have been cancelled somehow? Going to rerun the CI to see if it's temporary.

Docker image builds fine.
gcc 12, clang 14, clang 9 are all debug builds and do not use the feature.
The mingw builds do release builds and minetest builds fine.
The vs2019 builds do release builds and minetest builds fine.

It is notable that those CIs that create release builds also sometimes build the some dependencies. Here it would likely make sense to ensure that LTO is enabled for these too by modifying the build environment.

@tamara-schmitz
Copy link
Contributor Author

How do I rerun the pipeline? These two failures seem unrelated to the commit:
Screenshot of the build checks annotations

@ndren
Copy link
Contributor

ndren commented Feb 11, 2023

@tamara-schmitz If you send a new commit, it should run CI again. If you want to keep commit history clean, you could force push a new commit with the same changes and that should work as well. (Unfortunately, I don't think you can get CI to run again otherwise.)

@lhofhansl
Copy link
Contributor

Do you happen to have to performance test numbers? I'm curious about the magnitude of possible improvements.

@Zughy Zughy added the Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. label Feb 12, 2023
@ndren
Copy link
Contributor

ndren commented Mar 13, 2023

@lhofhansl No concrete numbers, but I am getting constant 60FPS now when it dropped below slightly before compiling in LTO, which is really nice since it means the CPU gets to sleep a lot more.

Also, it turns out that someone with write access to the repository can go to https://github.com/minetest/minetest/actions/runs/4118472326 and re-run CI: https://docs.github.com/en/actions/managing-workflow-runs/re-running-workflows-and-jobs

@sfan5 sfan5 added the Feature ✨ PRs that add or enhance a feature label Mar 22, 2023
@sfan5
Copy link
Member

sfan5 commented Mar 22, 2023

Sounds useful to me, though we should test this to make sure it doesn't expose unforeseen bugs.

Try force-pushing to rerun the pipelines.

@sfan5 sfan5 added Concept approved Approved by a core dev: PRs welcomed! and removed Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. labels Mar 22, 2023
@sfan5 sfan5 added this to the 5.8.0 milestone Mar 22, 2023
@tamara-schmitz
Copy link
Contributor Author

I have marked the tasks as complete since someone already ran these builds locally.

@rubenwardy rubenwardy added the WIP The PR is still being worked on by its author and not ready yet. label Jul 30, 2023
@Zughy
Copy link
Member

Zughy commented Jul 30, 2023

@tamara-schmitz is this still WIP?

@tamara-schmitz
Copy link
Contributor Author

@tamara-schmitz is this still WIP?

ready for merging imo. but im not a maintainer

@sfan5 sfan5 removed the WIP The PR is still being worked on by its author and not ready yet. label Jul 31, 2023
@sfan5 sfan5 self-requested a review July 31, 2023 12:08
@sfan5 sfan5 changed the title WIP: enable LTO building on supported platforms Enable LTO building on supported platforms by default Jul 31, 2023
@sfan5
Copy link
Member

sfan5 commented Jul 31, 2023

Do we need an extra option to disable this or does cmake already provide one?

@tamara-schmitz
Copy link
Contributor Author

Do we need an extra option to disable this or does cmake already provide one?

is a good idea to add that. gonna revise the patch

@Zughy Zughy changed the title Enable LTO building on supported platforms by default Draft: Enable LTO building on supported platforms by default Jul 31, 2023
@Zughy Zughy changed the title Draft: Enable LTO building on supported platforms by default Enable LTO building on supported platforms by default Jul 31, 2023
@Zughy Zughy marked this pull request as draft July 31, 2023 18:10
@Desour Desour added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Oct 1, 2023
@Desour
Copy link
Member

Desour commented Oct 1, 2023

@tamara-schmitz Are you still interested in this PR?
If yes, please add an option to en-/disable LTO (can be default on for release builds).

Do we need an extra option to disable this or does cmake already provide one?

I also quickly checked the answer for this. There seems to be none.
(There's CMAKE_INTERPROCEDURAL_OPTIMIZATION, but that enables it for all targets, and doesn't turn it off. And idk if you can set it from command line.)

@Desour Desour removed this from the 5.8.0 milestone Oct 1, 2023
@sfan5 sfan5 removed their request for review November 4, 2023 20:31
@okias
Copy link
Contributor

okias commented Dec 30, 2023

btw. default release builds should be definitely done with LTO. CI runs & stuff is not that great idea, because linking is much more expensive (and in case it's done, at least Mold or other performant linkers should be used).

@Desour
Copy link
Member

Desour commented Dec 30, 2023

Seems to be abandoned.

@Desour Desour closed this Dec 30, 2023
@Desour Desour added the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Dec 30, 2023
@okias okias mentioned this pull request Jan 1, 2024
1 task
@Zughy Zughy removed the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) @ Build CMake, build scripts, official builds, compiler and linker errors Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants