Disable IPO on all targets #3589

daschuer · 2021-01-24T00:56:52Z

Looking for a solution for this warning on windows,

LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/LTCG' specification

I have found that CMake has a generic solution for all targets, controlled by CMAKE_INTERPROCEDURAL_OPTIMIZATION

It turns out that IPO was only enabled for windows. I think If we want it enabled it should be done for all targets. However, I am not sure what it means for our build time and for the Mixx performance.

If it is only a matter of the size of the binary, we may consider to disable it for all.

What do you think?

On linux it add the options:
-flto -fno-fat-lto-objects

…targets if appropriated.

Be-ing · 2021-01-24T01:05:04Z

Do we plan to release with debug info? I think we should. In that case, this is somewhat pointless.

daschuer · 2021-01-24T02:51:43Z

now we have a new warning:
LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:ICF' specification

JoergAtGithub · 2021-01-24T05:35:14Z

For a local development environment incremental linking is what you need! For builds, you distribute, there is no benefit in incremental linked binaries, but they have a significiantly larger filesize.
On my old laptop repeated linking after small changes happens in seconds instead of minutes. The initial linking needs the same time.
To allow future incremental steps, no unused functions must be removed, because they might be needed in future. Therefore you have to disable file size optimisations like ICF and REF to use the incremental linker.

daschuer · 2021-01-24T08:25:08Z

@JoergAtGithub Thank you for the issue. What does it mean for the PDB file installer feature and Mixxx performance.

I want to make it possible that the user is able to create back-traces with source file line numbers from crash dumps or debug sessions.

This should not affect the performance of Mixxx.

The file size of the Mixxx binary is not that important Imho, the question is how the size of the distributes installer is affected and if the Required RAM changes.

Can you please check how the situation is for #3576
?
This is an IPO optimized build currently.

I can issue another PR with ipo disabled for compare.

daschuer · 2021-01-24T10:07:46Z

How does this tip relate to us?
https://forum.openframeworks.cc/t/faster-debugging-in-visual-studio-a-simple-trick/33779

JoergAtGithub · 2021-01-24T10:09:15Z

Debug symbols (PDB-files) should work independend of this. But in general if you use compiler optimization, it can be that code section are removed or are replaced by more efficient code. Than this code section will not occur in your stack trace.

With IPO, this optimizations will be done at much more places. This is because the optimization happens at link time instead of compile time, where the linker has the big picture.

In general there are three possibilities (each work with different optimization levels -Ox):
INCREMENTAL: Compile Time optimization (removal/inlining of whole functions happens only if they are local in a CPP file) -> Fastest build time (not for the initial build) big file size of executable
NORMAL: Compile Time optimization, linker removes unused functions -> Longer build time because the linker starts always from scratch. Small file size of executable
IPO: No compile time optimization, each CPP file is just translated 1:1 into the object files. Linker starts always from scratch and does the whole optmization -> Very long link time with very high memory usage. Executable is significiantly faster and slightly smaller than NORMAL.

JoergAtGithub · 2021-01-24T10:17:03Z

For local debug builds of Mixxx I change the following line in CMakeLists.txt:

This gives me build times of less than 20s for usual code changes, compared to some minutes with the default setup.

uklotzde · 2021-01-24T10:23:49Z

See also: https://github.com/rpmfusion/mixxx/blob/5eed41e7488b78b52ceb5de3397fd97e8152b992/mixxx.spec#L4

I will check with a local mock build if the situation has changed. Next release.

daschuer · 2021-01-24T11:25:00Z

Debian Focal:
15.690.084 Bytes IPO
16.766.874 Bytes No IPO
Binary:
10.528.376 Bytes IPO
13.475.320 Bytes No IPO
.text size
006d9a45 = 7.182.917 Bytes IPO
00948615 = 9.733.653 Bytes NO_IPO

daschuer · 2021-01-24T11:30:51Z

The IPO version runs without a crash on Ubuntu Focal

JoergAtGithub · 2021-01-24T11:45:18Z

In https://bugzilla.rpmfusion.org/show_bug.cgi?id=5829 it's written, that "even the unit tests are already crashing, although that previously worked".
If this is the case, I see no risk enable it in the Mixxx own CI builds, because such problems would become visible immediately.

daschuer · 2021-01-24T11:55:57Z

Ah OK, that's fortune.

So the remaining question is, do we get backtraces with line numbers if we enable IPO using the PDB file in the installer?
Can you verify:
#3576

Here is the alternative with IPO disabled:
#3590

If it works with IPO, i suggest to enable IPO for Release and RelWithDebInfo and disable it for Debug.
This avoids your manual /INCREMENTAL hack and saves some developer's time, while producing a squeezed binary for our releases.

daschuer · 2021-01-24T11:58:00Z

Is there than a point for "fastbuild" anymore?

JoergAtGithub · 2021-01-24T12:32:44Z

If it works with IPO, i suggest to enable IPO for Release and RelWithDebInfo and disable it for Debug.

This makes sense for me!

JoergAtGithub · 2021-01-24T14:56:14Z

Please remove the target "fastbuild" also from https://github.com/mixxxdj/mixxx/blob/main/tools/windows_buildenv.bat
And replace it in README.md

… IPO except for Debug Builds.

daschuer · 2021-01-24T16:03:03Z

Please remove the target "fastbuild" also from https://github.com/mixxxdj/mixxx/blob/main/tools/windows_buildenv.bat
And replace it in README.md

This is too hard for me without a windows machine.

"fastbuild" has now become "Native + Debug" this should produce the best developer experience.

Be-ing · 2021-01-24T16:09:54Z

CMakeLists.txt

+# CMAKE_INTERPROCEDURAL_OPTIMIZATION can be defined to override the default behaviour.
+# We keep DEFINED CMAKE_INTERPROCEDURAL_OPTIMIZATION unset = FALSE for Debug builds to save
+# build time during development at the cost of a bigger memory foodprint.
+# Note: This is has caused issues on Fedoa https://bugzilla.rpmfusion.org/show_bug.cgi?id=5829


Suggested change

# Note: This is has caused issues on Fedoa https://bugzilla.rpmfusion.org/show_bug.cgi?id=5829

# Note: This is has caused issues on Fedora https://bugzilla.rpmfusion.org/show_bug.cgi?id=5829

Be-ing · 2021-01-24T16:10:34Z

CMakeLists.txt

+# We keep DEFINED CMAKE_INTERPROCEDURAL_OPTIMIZATION unset = FALSE for Debug builds to save
+# build time during development at the cost of a bigger memory foodprint.
+# Note: This is has caused issues on Fedoa https://bugzilla.rpmfusion.org/show_bug.cgi?id=5829
+if(NOT DEFINED CMAKE_INTERPROCEDURAL_OPTIMIZATION AND NOT CMAKE_BUILD_TYPE STREQUAL "Debug" AND NOT OPTIMIZE STREQUAL "off")


Suggested change

if(NOT DEFINED CMAKE_INTERPROCEDURAL_OPTIMIZATION AND NOT CMAKE_BUILD_TYPE STREQUAL "Debug" AND NOT OPTIMIZE STREQUAL "off")

if(NOT DEFINED CMAKE_INTERPROCEDURAL_OPTIMIZATION AND NOT CMAKE_BUILD_TYPE STREQUAL "Debug" AND NOT OPTIMIZE)

If optimize is not set, CMake shoukd fail below. It defaults to "portable"

I guess using STREQUAL is correct here

@Holzhaus Your opinion?

I agree with @daschuer. Command line arguments in CMake have a type (you can check with cmake -L):

OPTIMIZE:STRING=portable

So we shouldn't mix different types and use STREQUAL throughout.

Be-ing · 2021-01-24T16:11:26Z

CMakeLists.txt

+
+# CMAKE_INTERPROCEDURAL_OPTIMIZATION can be defined to override the default behaviour.
+# We keep DEFINED CMAKE_INTERPROCEDURAL_OPTIMIZATION unset = FALSE for Debug builds to save
+# build time during development at the cost of a bigger memory foodprint.


Is the higher memory requirement at build time or run time? Please clarify the comment.

At run time.

CMakeLists.txt

Be-ing · 2021-01-24T16:13:35Z

Is the OPTIMIZE CMake option now effectively a binary? If so, let's treat it as such and use ON/OFF and get rid of the "portable" string.

daschuer · 2021-01-26T19:55:49Z

Done!

Co-authored-by: Be <be.0@gmx.com>

daschuer · 2021-01-27T21:52:46Z

Done

Be-ing · 2021-02-05T18:33:04Z

@JoergAtGithub:

In general there are three possibilities (each work with different optimization levels -Ox):
INCREMENTAL: Compile Time optimization (removal/inlining of whole functions happens only if they are local in a CPP file) -> Fastest build time (not for the initial build) big file size of executable
NORMAL: Compile Time optimization, linker removes unused functions -> Longer build time because the linker starts always from scratch. Small file size of executable
IPO: No compile time optimization, each CPP file is just translated 1:1 into the object files. Linker starts always from scratch and does the whole optmization -> Very long link time with very high memory usage. Executable is significiantly faster and slightly smaller than NORMAL.

So maybe only use IPO for Release build type and everything else use INCREMENTAL? Then we could do RelWithDebInfo builds on GH Actions and cache the CMake build directory since we haven't gotten clcache to work.

daschuer · 2021-02-05T19:56:18Z

We can ignore "Release" because we need the debug info for the symbol files on each of our targets.

The idea was to use Debug for development and RelWithDebInfo for our releases.

I am in doubt that IPO makes Mixxx faster. It removes anyway unused .text segment. That will improve the start up and limit the memory footprint. However the .text segment is small compared to the heap we use.

See here:
#3589 (comment)

I don't mind if we disable Ipo always.
If one needs it because of a tiny flash disk on an embedded device or something, he can use the standard CMake variables to activate it.

JoergAtGithub · 2021-02-05T20:29:56Z

IMHO IPO should be enabled for the official release builds and INCREMENTAL (default) for local development builds. For the GitHub CI builds, we should use, whatever provides the fastest build times.

daschuer · 2021-02-05T20:47:12Z

That using a different compilers settings for CI than release is IMHO not a good idea because, than we will release an almost untested release.

Do you have measures that reveals difference s in run-time speed?

uklotzde · 2021-02-22T21:53:18Z

Please resolve the minor conflicts.

Anything else to do here?

daschuer · 2021-02-22T22:23:39Z

The PR is done. The useful part is at least that IPO is no accessible as a cmake parameter.

I am really not sure if the extra CPU time outweighs the smaller binary.

Currently IPO is enabled for RelWithdebInfo builds, we want to release.
We can easily change that if the CI build are annoying long now.

Be-ing · 2021-02-24T18:43:58Z

I am really not sure if the extra CPU time outweighs the smaller binary.

I don't think the extra CPU time on CI is worth the tradeoff. The Ubuntu 18.04 build took 29m 53s with a compiler cache. It normally takes ~5 minutes. So I think we should close this without merging.

Run ccache -s
cache directory                     /home/runner/.ccache
primary config                      /home/runner/.ccache/ccache.conf
secondary config      (readonly)    /etc/ccache.conf
stats zero time                     Mon Feb 22 20:04:53 2021
cache hit (direct)                  9654
cache hit (preprocessed)               3
cache miss                          1926
cache hit rate                     83.37 %
called for link                      570
cleanups performed                     0
files in cache                      3497
cache size                           2.4 GB
max cache size                  2097152.0 GB

daschuer · 2021-02-24T20:17:29Z

Did you compare two consecutive builds?
The ccache has no effect if you change the compiler flags.

I think we should merge this at any case, because it equalizes the settings of all our tragets.
The settings for our ci and other he default settings can be tweaked at any time based on this.

Be-ing · 2021-02-24T20:18:45Z

Did you compare two consecutive builds?

Yes, I posted the ccache -s output verifying that the cache was hit.

Be-ing · 2021-02-24T20:23:04Z

The macOS build took 1 hour with cache hits 😬

Run ccache -s
cache directory                     /Users/runner/Library/Caches/ccache
primary config                      /Users/runner/Library/Preferences/ccache/ccache.conf
secondary config (readonly)         /Users/mixxx/bs-2.3-mac/amd64/environment/2.3-j00026-0b1209e3-osx10.12-x86_64-release/etc/ccache.conf
stats updated                       Wed Feb 24 18:53:24 2021
cache hit (direct)                  9067
cache hit (preprocessed)               9
cache miss                          1931
cache hit rate                     82.46 %
called for link                      588
cleanups performed                     0
files in cache                      3529
cache size                           2.6 GB
max cache size                  2097152.0 GB

Maybe IPO is why we haven't gotten clcache nor sccache to work?

daschuer · 2021-02-24T20:27:46Z

How many previous builds are covered by ccache -s?

Be-ing · 2021-02-24T20:29:02Z

Good question. I manually reran the last builds to double check. Let's see how it goes...

Be-ing · 2021-02-24T21:21:09Z

Repeat build times:
Ubuntu 18.04: 10m 19s
Ubuntu 20.04: 18m 27s
macOS: 34m 44s
Windows: 39m 38s

IMO the increased build time for macOS makes this not worth it.

…ment for documentation purpose.

daschuer · 2021-02-25T07:27:32Z

IPO is now disabled, the comments are kept for future reference.

Be-ing · 2021-02-25T17:30:18Z

Windows build is down to 32 minutes without caching. 👍

Be-ing · 2021-02-25T19:00:19Z

I don't think the compiler flags are getting applied as intended. Look at the sccache log in https://github.com/mixxxdj/mixxx/runs/1981578442

daschuer · 2021-02-25T21:07:50Z

What issue do you see?
For wnumberpos.cpp for instance I see during the compile call:
/W3 /GR /EHsc /MD /Zi /O2 /Ob1 /DNDEBUG /fp:fast /Gy /W3

daschuer · 2021-02-25T21:18:50Z

Did you expect to see /Z7? You don't see it, because you have added you patch into the optimize=off region.

Be-ing · 2021-02-25T21:29:48Z

Oh, good catch.

Be-ing · 2021-02-26T07:51:09Z

There is now a warning on Windows:

LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:ICF' specification

Make use of CMAKE_INTERPROCEDURAL_OPTIMIZATION and enable it for all …

c87ac83

…targets if appropriated.

github-actions bot added the build label Jan 24, 2021

Allow to override CMAKE_INTERPROCEDURAL_OPTIMIZATION

20c89c8

daschuer force-pushed the ipo_on branch from fc1dbcf to 5a3ffae Compare January 24, 2021 10:33

daschuer force-pushed the ipo_on branch from 5a3ffae to 08b1de0 Compare January 24, 2021 14:50

daschuer force-pushed the ipo_on branch 2 times, most recently from e675de0 to b5d9531 Compare January 24, 2021 15:18

Dispose OPTIMIZE="fastbuild" Deal with CMake default flags and enable…

1af3d88

… IPO except for Debug Builds.

daschuer force-pushed the ipo_on branch from b5d9531 to 1af3d88 Compare January 24, 2021 15:40

Be-ing reviewed Jan 24, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Holzhaus mentioned this pull request Jan 27, 2021

use Windows dependencies from vcpkg #3594

Closed

Fix typo.

0959884

Co-authored-by: Be <be.0@gmx.com>

Merge remote-tracking branch 'upstream/2.3' into ipo_on

e77ac2c

disble the CMAKE_INTERPROCEDURAL_OPTIMIZATION code but keep it as com…

1c49c0d

…ment for documentation purpose.

daschuer changed the title ~~Enable IPO on all targets~~ Disable IPO on all targets Feb 25, 2021

Be-ing merged commit b0103a4 into mixxxdj:2.3 Feb 25, 2021

Be-ing mentioned this pull request Feb 25, 2021

use sccache on Windows #3618

Merged

daschuer deleted the ipo_on branch September 26, 2021 17:42

	# Note: This is has caused issues on Fedoa https://bugzilla.rpmfusion.org/show_bug.cgi?id=5829
	# Note: This is has caused issues on Fedora https://bugzilla.rpmfusion.org/show_bug.cgi?id=5829

	if(NOT DEFINED CMAKE_INTERPROCEDURAL_OPTIMIZATION AND NOT CMAKE_BUILD_TYPE STREQUAL "Debug" AND NOT OPTIMIZE STREQUAL "off")
	if(NOT DEFINED CMAKE_INTERPROCEDURAL_OPTIMIZATION AND NOT CMAKE_BUILD_TYPE STREQUAL "Debug" AND NOT OPTIMIZE)

Disable IPO on all targets #3589

Disable IPO on all targets #3589

Conversation

daschuer commented Jan 24, 2021

Be-ing commented Jan 24, 2021

daschuer commented Jan 24, 2021

JoergAtGithub commented Jan 24, 2021

daschuer commented Jan 24, 2021

daschuer commented Jan 24, 2021

JoergAtGithub commented Jan 24, 2021

JoergAtGithub commented Jan 24, 2021

uklotzde commented Jan 24, 2021 • edited Loading

daschuer commented Jan 24, 2021 • edited Loading

daschuer commented Jan 24, 2021

JoergAtGithub commented Jan 24, 2021

daschuer commented Jan 24, 2021

daschuer commented Jan 24, 2021

JoergAtGithub commented Jan 24, 2021 • edited Loading

JoergAtGithub commented Jan 24, 2021

daschuer commented Jan 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Jan 24, 2021

daschuer commented Jan 26, 2021

daschuer commented Jan 27, 2021

Be-ing commented Feb 5, 2021

daschuer commented Feb 5, 2021

JoergAtGithub commented Feb 5, 2021

daschuer commented Feb 5, 2021 • edited Loading

uklotzde commented Feb 22, 2021

daschuer commented Feb 22, 2021

Be-ing commented Feb 24, 2021

daschuer commented Feb 24, 2021 • edited Loading

Be-ing commented Feb 24, 2021

Be-ing commented Feb 24, 2021

daschuer commented Feb 24, 2021

Be-ing commented Feb 24, 2021

Be-ing commented Feb 24, 2021

daschuer commented Feb 25, 2021

Be-ing commented Feb 25, 2021

Be-ing commented Feb 25, 2021

daschuer commented Feb 25, 2021

daschuer commented Feb 25, 2021

Be-ing commented Feb 25, 2021

Be-ing commented Feb 26, 2021

uklotzde commented Jan 24, 2021 •

edited

Loading

daschuer commented Jan 24, 2021 •

edited

Loading

JoergAtGithub commented Jan 24, 2021 •

edited

Loading

daschuer commented Feb 5, 2021 •

edited

Loading

daschuer commented Feb 24, 2021 •

edited

Loading