Enable IPO/LTO builds #14198

okias · 2024-01-01T15:06:42Z

Enabling IPO/LTO leads to faster loading times, better performance, more efficient cache usage, and a reduction in binaries size.

Cons: Release build compilation takes longer and consumes more energy (thou disabling it for CI builds makes sense).

Only DEBUG build default setting is OFF, otherwise it's ON by default.

$ cmake . -DRUN_IN_PLACE=TRUE -DCMAKE_BUILD_TYPE=Release -DBUILD_SERVER=TRUE -DENABLE_TOUCH=FALSE

# binary sizes:
         minetest minetestserver
W/o LTO:      13M           7.3M
W/  LTO:      11M           5.9M
difference:   15%            19%

Different version of abandoned #13192

TODO:

disable ENABLE_LTO for CI builds.

Desour · 2024-01-01T16:51:15Z

Not sure, but I guess the ODR violation might be there because irrlichtmt is compiled with -fno-rtti, so the rtti is included in minetest's translation units' vtables but in not irrlichtmt ones'.
Idk why we use -fno-rtti, we probably shouldn't.

okias · 2024-01-01T18:15:09Z

Idk why we use -fno-rtti, we probably shouldn't.

The value of using LTO vs using -fno-rtti on Irrlicht only is not worth it, so yes. Dropping -no-rtti make sense there.

PR removing -fno-rtti from Irrlicht minetest/irrlicht#268

lhofhansl · 2024-01-06T00:31:44Z

Nice.
I don't see any runtime performance improvements, but the executables are smaller as expected.

okias · 2024-01-07T00:07:23Z

For now, I have to skip WIN32, because it has random weird failures (like wchar, wstring 4 vs 2 bytes, and other stuff), but if anyone wants to play with it, maybe will be enabling the LTO option too there.

Desour

I'd like to be able to enable LTO for non-Release builds too, e.g. just for getting more warnings, and also for RelWithDebInfo.
Can we have an option which just has a different default depending on the build type?

src/client/particles.cpp

Desour · 2024-01-27T20:35:07Z

Right now, LTO is always default ON. Only exception is Debug build where it's default off.

Correct me if I'm wrong, but isn't CMAKE_INTERPROCEDURAL_OPTIMIZATION_RELEASE only used by cmake if build type is Release?
How can I check whether an executable was built with LTO?

okias · 2024-01-28T13:58:04Z

Correct me if I'm wrong, but isn't CMAKE_INTERPROCEDURAL_OPTIMIZATION_RELEASE only used by cmake if build type is Release? How can I check whether an executable was built with LTO?

Thanks, fixed.

Generally the size difference should tell you in 99.99% cases.

Desour

New build option needs documentation in doc/compiling/README.md.
I'm getting another warning in nodedef.cpp:737. (At least after a trivial rebase.)
The default value is debatable. On my machine it takes about 2min with RelWithDebInfo and mold linker.

CMakeLists.txt

src/gui/guiTable.cpp

src/script/lua_api/l_server.cpp

Test case: ``` $ cmake . -DRUN_IN_PLACE=TRUE -DCMAKE_BUILD_TYPE=Release -DBUILD_SERVER=TRUE -DENABLE_TOUCH=FALSE minetest minetestserver W/o LTO: 13M 7.3M W/ LTO: 11M 5.9M difference: 15% 19% ``` Signed-off-by: David Heidelberg <david@ixit.cz>

Take care about possible allocation of too large array (not likely to happen in real life (TM)). ``` /home/okias/projects/minetest/src/gui/guiTable.cpp:239:45: warning: argument 1 value ‘18446744073709551615’ exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=] 239 | TempRow *rows = new TempRow[rowcount]; | ^ ``` Signed-off-by: David Heidelberg <david@ixit.cz>

Compiler is afraid that variables will be uninitialized, but when is code around, he seems to be happy. Signed-off-by: David Heidelberg <david@ixit.cz>

Signed-off-by: David Heidelberg <david@ixit.cz>

In CI, linking takes extra time and rarely causes regressions. Signed-off-by: David Heidelberg <david@ixit.cz>

Signed-off-by: David Heidelberg <david@ixit.cz>

okias · 2024-02-07T08:34:08Z

New build option needs documentation in doc/compiling/README.md.

Done.

I'm getting another warning in nodedef.cpp:737. (At least after a trivial rebase.)

Initialized.

The default value is debatable. On my machine it takes about 2min with RelWithDebInfo and mold linker.

Sure, sure, thou for release it's always "right thing to do", for other builds I would say it's for consideration depending on the machine, energy cost and patience 😉 😆

Desour

Thanks!

(Didn't test on anything other than linux x86-64 gcc mold.)

appgurueu · 2024-02-07T19:49:44Z

Tested on linux x86-64 gcc as well, played a bit of CTF as a "smoke test". I got the following warning when building:

lto-wrapper: warning: using serial compilation of 126 LTRANS jobs

but this seems to be a non-issue.

One minor annoyance: set writes to the cache. So after building a release build (cmake . -DCMAKE_BUILD_TYPE=Release), IPO will be enabled even if I switch to a debug build (cmake . -DCMAKE_BUILD_TYPE=Debug) and vice versa; I have to explicitly set or unset it to get rid of this behavior. It might be better if we could store "explicitly enabled, explicitly disabled, or absent" (basically an "optional bool") in the cache somehow, though I'm not sure how that would be best mapped to CMake. I don't think this is a big issue though, since (1) other flags do the same, (2) a clean build doesn't have the problem anyways, (3) worst case, a debug build accidentally uses LTO, making it slightly slower.

okias · 2024-02-08T18:36:00Z

@appgurueu I could drop the CACHE for the option, anyway I agree it's not big deal :D

sfan5 · 2024-02-12T19:50:57Z

I wanted to to note that the errors with LTO you saw on win32:

/usr/x86_64-w64-mingw32/bin/ld: scripting_client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD1Ev[_ZTv0_n24_N15ClientScriptingD1Ev]+0x0): multiple definition of `ClientScripting::~ClientScripting()'; client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD1Ev[_ZThn8_N15ClientScriptingD1Ev]+0x0): first defined here
/usr/x86_64-w64-mingw32/bin/ld: scripting_client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD1Ev[_ZTv0_n24_N15ClientScriptingD1Ev]+0x0): multiple definition of `virtual thunk to ClientScripting::~ClientScripting()'; client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD1Ev[_ZThn8_N15ClientScriptingD1Ev]+0x0): first defined here
/usr/x86_64-w64-mingw32/bin/ld: scripting_client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD1Ev[_ZTv0_n24_N15ClientScriptingD1Ev]+0x0): multiple definition of `non-virtual thunk to ClientScripting::~ClientScripting()'; client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD1Ev[_ZThn8_N15ClientScriptingD1Ev]+0x0): first defined here
/usr/x86_64-w64-mingw32/bin/ld: scripting_client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD1Ev[_ZTv0_n24_N15ClientScriptingD1Ev]+0x0): multiple definition of `non-virtual thunk to ClientScripting::~ClientScripting()'; client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD1Ev[_ZThn8_N15ClientScriptingD1Ev]+0x0): first defined here
/usr/x86_64-w64-mingw32/bin/ld: scripting_client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD0Ev[_ZTv0_n24_N15ClientScriptingD0Ev]+0x0): multiple definition of `ClientScripting::~ClientScripting()'; client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD0Ev[_ZThn8_N15ClientScriptingD0Ev]+0x0): first defined here
/usr/x86_64-w64-mingw32/bin/ld: scripting_client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD0Ev[_ZTv0_n24_N15ClientScriptingD0Ev]+0x0): multiple definition of `virtual thunk to ClientScripting::~ClientScripting()'; client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD0Ev[_ZThn8_N15ClientScriptingD0Ev]+0x0): first defined here
/usr/x86_64-w64-mingw32/bin/ld: scripting_client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD0Ev[_ZTv0_n24_N15ClientScriptingD0Ev]+0x0): multiple definition of `non-virtual thunk to ClientScripting::~ClientScripting()'; client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD0Ev[_ZThn8_N15ClientScriptingD0Ev]+0x0): first defined here
/usr/x86_64-w64-mingw32/bin/ld: scripting_client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD0Ev[_ZTv0_n24_N15ClientScriptingD0Ev]+0x0): multiple definition of `non-virtual thunk to ClientScripting::~ClientScripting()'; client.cpp.obj (symbol from plugin):(.gnu.linkonce.t._ZN15ClientScriptingD0Ev[_ZThn8_N15ClientScriptingD0Ev]+0x0): first defined here
[...]

might be a bug in GCC as someone has reported here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94156

Test case: ``` $ cmake . -DRUN_IN_PLACE=TRUE -DCMAKE_BUILD_TYPE=Release -DBUILD_SERVER=TRUE -DENABLE_TOUCH=FALSE minetest minetestserver W/o LTO: 13M 7.3M W/ LTO: 11M 5.9M difference: 15% 19% ``` Also fixes various compiler warnings resulting from compilation using LTO. --------- Signed-off-by: David Heidelberg <david@ixit.cz>

This comment was marked as outdated.

Sign in to view

Desour added @ Build CMake, build scripts, official builds, compiler and linker errors Performance labels Jan 1, 2024

okias force-pushed the enable-lto branch 3 times, most recently from 68264f3 to 6334b42 Compare January 6, 2024 23:53

okias marked this pull request as ready for review January 7, 2024 00:29

Desour requested changes Jan 22, 2024

View reviewed changes

src/client/particles.cpp Outdated Show resolved Hide resolved

Desour added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jan 22, 2024

Desour self-assigned this Jan 22, 2024

okias force-pushed the enable-lto branch from 0c67e57 to 9385712 Compare January 26, 2024 02:19

okias force-pushed the enable-lto branch from 9385712 to b92f951 Compare January 28, 2024 12:40

Desour requested changes Jan 29, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

src/gui/guiTable.cpp Outdated Show resolved Hide resolved

src/script/lua_api/l_server.cpp Outdated Show resolved Hide resolved

okias added 9 commits February 7, 2024 00:39

fixup! Enable IPO/LTO by default except for debug builds

b872704

server: move the RTT and JITTER info closer to the LUA counterpart

8cc8bd3

Compiler is afraid that variables will be uninitialized, but when is code around, he seems to be happy. Signed-off-by: David Heidelberg <david@ixit.cz>

particles: frame_count is unused in this call, drop it

fcd6ac6

Signed-off-by: David Heidelberg <david@ixit.cz>

ci: disable LTO builds

cac4865

In CI, linking takes extra time and rarely causes regressions. Signed-off-by: David Heidelberg <david@ixit.cz>

particles: Initialize frame_length_i

92a6be1

Signed-off-by: David Heidelberg <david@ixit.cz>

fixes to squash

39eceb2

Signed-off-by: David Heidelberg <david@ixit.cz>

fixup! particles: Initialize frame_length_i

1b164f7

okias force-pushed the enable-lto branch from b92f951 to 1b164f7 Compare February 6, 2024 23:44

sync CMake requirement with Irrlicht

74aaf3c

Signed-off-by: David Heidelberg <david@ixit.cz>

okias requested a review from Desour February 7, 2024 08:30

Desour approved these changes Feb 7, 2024

View reviewed changes

Desour added One approval ✅ ◻️ and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Feb 7, 2024

okias requested a review from grorp February 7, 2024 12:08

okias changed the title ~~Enable IPO/LTO for production builds~~ Enable IPO/LTO builds Feb 7, 2024

appgurueu added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Feb 7, 2024

appgurueu merged commit eb52a14 into minetest:master Feb 8, 2024
13 checks passed

okias deleted the enable-lto branch February 9, 2024 12:55

Niklp09 mentioned this pull request Feb 9, 2024

Weird raycast performance degradation since #14198 #14362

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable IPO/LTO builds #14198

Enable IPO/LTO builds #14198

okias commented Jan 1, 2024 •

edited

This comment was marked as outdated.

Desour commented Jan 1, 2024

okias commented Jan 1, 2024

lhofhansl commented Jan 6, 2024

okias commented Jan 7, 2024

Desour left a comment

Desour commented Jan 27, 2024

okias commented Jan 28, 2024 •

edited

Desour left a comment

okias commented Feb 7, 2024 •

edited

Desour left a comment

appgurueu commented Feb 7, 2024

okias commented Feb 8, 2024

sfan5 commented Feb 12, 2024

Enable IPO/LTO builds #14198

Enable IPO/LTO builds #14198

Conversation

okias commented Jan 1, 2024 • edited

This comment was marked as outdated.

Desour commented Jan 1, 2024

okias commented Jan 1, 2024

lhofhansl commented Jan 6, 2024

okias commented Jan 7, 2024

Desour left a comment

Choose a reason for hiding this comment

Desour commented Jan 27, 2024

okias commented Jan 28, 2024 • edited

Desour left a comment

Choose a reason for hiding this comment

okias commented Feb 7, 2024 • edited

Desour left a comment

Choose a reason for hiding this comment

appgurueu commented Feb 7, 2024

okias commented Feb 8, 2024

sfan5 commented Feb 12, 2024

okias commented Jan 1, 2024 •

edited

okias commented Jan 28, 2024 •

edited

okias commented Feb 7, 2024 •

edited