-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[build] Enable link-time optimization for iOS release builds #12502
Conversation
@jfirebaugh Nice. A few questions:
|
ThinLTO compiles successfully, but reduces the size decrease between 0.5 and 2 percentage points, so let's stick with "monolithic" LTO.
Yes, it seems to be faster across the board:
Before:
After:
Yes, the link step takes noticeably longer. However, since we're enabling this only for release builds, this will not affect local development in the common case. |
@@ -3781,6 +3781,7 @@ | |||
INFOPLIST_FILE = framework/Info.plist; | |||
INSTALL_PATH = "$(LOCAL_LIBRARY_DIR)/Frameworks"; | |||
LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; | |||
LLVM_LTO = YES_THIN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This enables LTO for the dynamic
target — we’ll also want to do this for the static
target.
platform/ios/config.cmake
Outdated
set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -flto=thin") | ||
set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO} -flto=thin") | ||
set(CMAKE_EXE_LINKER_FLAGS_RELEASE "${CMAKE_EXE_LINKER_FLAGS_RELEASE} -flto=thin") | ||
set(CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO "${CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO} -flto=thin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consult @kkaefer, but I think using our set_xcode_property
macro to set Xcode’s LLVM_LTO
flag will be easier to read in the cmake config and clearer when looking at the settings in Xcode itself.
set_xcode_property(${target} LLVM_LTO $<$<CONFIG:Release>:YES>)
set_xcode_property(${target} LLVM_LTO $<$<CONFIG:RelWithDebugInfo>:YES>)
Or we could combine the two build modes into a single line using a more complex/ugly generator expression:
set_xcode_property(${target} LLVM_LTO $<$<OR:$<CONFIG:Release>,$<CONFIG:RelWithDebugInfo>>:YES>)
This should have the same effect as -flto
and has the benefit of being reflected in the Build Settings UI:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for most targets, but not for mbgl-node
because it's an "INTERFACE_LIBRARY" target:
https://circleci.com/gh/mapbox/mapbox-gl-native/136552
Should we:
- Not bother with LTO for macOS node bindings
- Use
set_xcode_property
ininitialize_xcode_cxx_build_settings
instead - Loop with
foreach(ABI IN LISTS mbgl-node::abis)
in macos/config.cmake andset_xcode_property
for each (non-INTERFACE_LIBRARY) target
?
platform/ios/config.cmake
Outdated
set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -flto=thin") | ||
set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO} -flto=thin") | ||
set(CMAKE_EXE_LINKER_FLAGS_RELEASE "${CMAKE_EXE_LINKER_FLAGS_RELEASE} -flto=thin") | ||
set(CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO "${CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO} -flto=thin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should also note that this config doesn’t affect the mbgl targets in the macos project, so those currently don’t have LTO set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, ideally we'd set the Xcode configuration settings. However, technically you could also run cmake .. -GNinja
on macOS to build on macOS without Xcode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙇
This results in a substantial size savings.
I haven't been able to get LTO working for Android builds due to android/ndk#721. I'll open a followup ticket for that.