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

Actually compile with proper optimizations in a release build. #6336

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

Conversation

BMagnu
Copy link
Member

@BMagnu BMagnu commented Sep 4, 2024

As it turns out, we did not actually compile the release builds with the maximum safe optimization level.
First of all, on Linux / Mac builds, we did not compile with -O3, but only with -O2. That's wasted performance, as O3 is still safe (unlike -Ofast, which may do inaccurate math).
Second of all, none of the toolchains has had Link-Time Optimizations enabled. This is especially bad in the FSO case, as we have some functions in cpp's (and thus uninlineable unless LTO is available) which are called very often (such as the vector math functions).
Enabling LTO throws up a few warnings: Some ODR violations, two uninitialized structs, and a few "potential negative size allocations". These latter ones are technically already caught with an Assertion, but as these are not compiled into release builds, the compiler sees a potentially dangerous path and complains. Until we get assumptions for the compiler (which is not soon, given that these are compiler extensions added to GCC13+ and standardized in C++23), we have to just make sure that the code at runtime ensures their correctness, even if that may be redundant.
Furthermore, an update to CMake 3.9 is required to actually get portable LTO support detection and enabling.
This also yields actually measureable performance increases.
In a particle stress test I was profiling (which prompted this PR), a performance boost of 10% is detectable (lower single graph / orange line is with LTO and -O3):
image2

@BMagnu BMagnu added build An issue related to the build systems Waiting for Stable Marks a pull request that is to be merged after the next stable release, due to a release cycle labels Sep 4, 2024
@BMagnu
Copy link
Member Author

BMagnu commented Sep 4, 2024

There seem to be some stray warnings left that my local GCC did not trigger, I'll need to fix them before this is merged / reviewed

@BMagnu BMagnu marked this pull request as draft September 4, 2024 14:04
@EatThePath
Copy link
Contributor

Oh, nice find. Have you measured any performance change in more gamelike conditions than that stress test?

@BMagnu
Copy link
Member Author

BMagnu commented Sep 4, 2024

I want to do icarus at some point, but I didn't find the time yet. I'll probably get to it around the weekend, once I squashed the remaining warnings.
And, evidently, I can only speak for the linux performance, not for how much LTO actually does for MSVC here.

@BMagnu BMagnu force-pushed the compiler_settings branch 2 times, most recently from bf062bd to 11b2932 Compare September 5, 2024 14:10
@BMagnu BMagnu marked this pull request as ready for review September 5, 2024 14:50
@BMagnu
Copy link
Member Author

BMagnu commented Sep 6, 2024

Okay, finally managed to benchmark Icarus. Upper Graph / Blue Line is master, Lower Graph / Orange is this PR.
image
As such, a small but consistent speedup can be observed.
Of course, this is quite compiler-dependent, so the actual speedup observed on MSVC may be different.
It is also to note, that the main bottleneck on my system is the very moody nvidia driver. As such, stuff like system / driver uptime has an effect on my benchmarks that far exceeds most changes (i.e. rebooting my system after a day of use usually nets +25% FPS with the nvidia driver). As such, take any benchmarks from me with some grain of salt when comparing it to others not directly measured at the same time.

@Goober5000 Goober5000 removed the Waiting for Stable Marks a pull request that is to be merged after the next stable release, due to a release cycle label Oct 28, 2024
@BMagnu BMagnu mentioned this pull request Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build An issue related to the build systems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants