build(core): Enable link-time optimization for improved runtime performance. #1104
Conversation
|
""" WalkthroughThe CMakeLists.txt file in the core components was updated to add support for link-time optimization (LTO) by checking for interprocedural optimization (IPO) support and enabling it if available only for Release builds. Status messages indicate whether LTO is enabled. No other logic or public interfaces were changed. Changes
Possibly related issues
Suggested reviewers
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningscomponents/core/CMakeLists.txt (14)Learnt from: gibber9809 Learnt from: Bill-hbrhbr Learnt from: sitaowang1998 Learnt from: LinZhihao-723 Learnt from: LinZhihao-723 Learnt from: jackluo923 Learnt from: jackluo923 Learnt from: jackluo923 Learnt from: Bill-hbrhbr Learnt from: haiqi96 Learnt from: LinZhihao-723 Learnt from: Bill-hbrhbr Learnt from: AVMatthews Learnt from: jackluo923 ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Learnt from: LinZhihao-723
PR: y-scope/clp#873
File: components/core/src/clp/ffi/ir_stream/search/test/utils.hpp:15-18
Timestamp: 2025-05-02T22:11:55.711Z
Learning: The team plans to systematically improve include paths by adding `${PROJECT_SOURCE_DIR}/src` to CMake targets and using direct include paths instead of deep relative ones in a future PR.
Learnt from: LinZhihao-723
PR: y-scope/clp#549
File: components/core/tests/test-ir_encoding_methods.cpp:1180-1186
Timestamp: 2024-10-01T07:59:11.208Z
Learning: In the context of loop constructs, LinZhihao-723 prefers using `while (true)` loops and does not consider alternative loop constructs necessarily more readable.
Learnt from: LinZhihao-723
PR: y-scope/clp#549
File: components/core/tests/test-ir_encoding_methods.cpp:1180-1186
Timestamp: 2024-10-08T15:52:50.753Z
Learning: In the context of loop constructs, LinZhihao-723 prefers using `while (true)` loops and does not consider alternative loop constructs necessarily more readable.
components/core/CMakeLists.txt (4)
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Learnt from: LinZhihao-723
PR: y-scope/clp#873
File: components/core/src/clp/ffi/ir_stream/search/test/utils.hpp:15-18
Timestamp: 2025-05-02T22:11:55.711Z
Learning: The team plans to systematically improve include paths by adding `${PROJECT_SOURCE_DIR}/src` to CMake targets and using direct include paths instead of deep relative ones in a future PR.
Learnt from: anlowee
PR: y-scope/clp#925
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-19
Timestamp: 2025-05-26T18:39:51.727Z
Learning: In CMakeLists.txt files, anlowee prefers to explicitly list source files one by one rather than using file(GLOB ...) or file(GLOB_RECURSE ...) to automatically include files, maintaining consistency with other CMake files in the project.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build (macos-latest)
There was a problem hiding this comment.
Can we resturture the LTO optimization as 'release' build opt-in feature? i.e. LTO is enabled or no-op rather than LTO is enabled or disabled. Rigjt now, functionally it's opt-in based solution, but in the log message, we are still suggesting LTO is a feature we disable.
|
Can we add some details about the microbenchmark performed? |
Added in the PR description. |
I'm not sure I fully understand what u suggest. The reason to "disable" it for non-release build is to avoid paying longer build time when it's unnecessary. At this point I don't see a need to allow users to turn it on/off, since cmake hides the underlying compiler details and the compiler may not even support LTO when users enable it. |
I think (I might be wrong) Jack means that it's odd to print a warning or status message when LTO is not enabled, since the only reason it wouldn't enabled is because the processor doesn't support it or it's not turned on for debug builds. I kind of agree in that regard. It's probably clearer to print a message only when it's turned on. |
Sure. I guess I should drop the two "not enabled" messages then. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Learnt from: gibber9809
PR: y-scope/clp#504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Learnt from: LinZhihao-723
PR: y-scope/clp#873
File: components/core/src/clp/ffi/ir_stream/search/test/utils.hpp:15-18
Timestamp: 2025-05-02T22:11:55.711Z
Learning: The team plans to systematically improve include paths by adding `${PROJECT_SOURCE_DIR}/src` to CMake targets and using direct include paths instead of deep relative ones in a future PR.
Learnt from: anlowee
PR: y-scope/clp#925
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-19
Timestamp: 2025-05-26T18:39:51.727Z
Learning: In CMakeLists.txt files, anlowee prefers to explicitly list source files one by one rather than using file(GLOB ...) or file(GLOB_RECURSE ...) to automatically include files, maintaining consistency with other CMake files in the project.
Learnt from: LinZhihao-723
PR: y-scope/clp#549
File: components/core/tests/test-ir_encoding_methods.cpp:1180-1186
Timestamp: 2024-10-01T07:59:11.208Z
Learning: In the context of loop constructs, LinZhihao-723 prefers using `while (true)` loops and does not consider alternative loop constructs necessarily more readable.
Learnt from: LinZhihao-723
PR: y-scope/clp#549
File: components/core/tests/test-ir_encoding_methods.cpp:1180-1186
Timestamp: 2024-10-08T15:52:50.753Z
Learning: In the context of loop constructs, LinZhihao-723 prefers using `while (true)` loops and does not consider alternative loop constructs necessarily more readable.
components/core/CMakeLists.txt (10)
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Learnt from: LinZhihao-723
PR: y-scope/clp#873
File: components/core/src/clp/ffi/ir_stream/search/test/utils.hpp:15-18
Timestamp: 2025-05-02T22:11:55.711Z
Learning: The team plans to systematically improve include paths by adding `${PROJECT_SOURCE_DIR}/src` to CMake targets and using direct include paths instead of deep relative ones in a future PR.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:331-335
Timestamp: 2024-10-14T03:45:21.323Z
Learning: When reviewing code, ensure that suggestions about unnecessary use of `std::move` with `std::shared_ptr` are only made if `std::move` is actually used in the code.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-03-18T07:27:54.738Z
Learning: Double parentheses in the codebase are intentional and required for clang-tidy to pass. These should not be flagged as style issues in code reviews.
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.
Learnt from: LinZhihao-723
PR: y-scope/clp#557
File: components/core/tests/test-ir_encoding_methods.cpp:1216-1286
Timestamp: 2024-10-13T09:27:43.408Z
Learning: In the unit test case `ffi_ir_stream_serialize_schema_tree_node_id` in `test-ir_encoding_methods.cpp`, suppressing the `readability-function-cognitive-complexity` warning is acceptable due to the expansion of Catch2 macros in C++ tests, and such test cases may not have readability issues.
components/core/CMakeLists.txt
Outdated
| if(CMAKE_BUILD_TYPE MATCHES "Release") | ||
| include(CheckIPOSupported) | ||
| check_ipo_supported(RESULT IPO_SUPPORTED OUTPUT IPO_CHECK_MSG) | ||
| if(IPO_SUPPORTED) | ||
| message(STATUS "Link-time optimization enabled.") | ||
| set(CMAKE_INTERPROCEDURAL_OPTIMIZATION ON) | ||
| endif() | ||
| else() | ||
|
|
There was a problem hiding this comment.
else() branch never closed – CMake will fail to configure
if(CMAKE_BUILD_TYPE MATCHES "Release") opens at L24 and its else() opens at L31, but the corresponding endif() for the outer if() is missing. This breaks block balance and will trigger:
CMake Error: Parse error in command while processing CMakeLists.txt
Add the missing terminator right after the non-Release branch contents:
@@
-else()
-
-set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
+# Non-Release builds
+else()
+ set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
+endif() # CMAKE_BUILD_TYPE MATCHES "Release"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(CMAKE_BUILD_TYPE MATCHES "Release") | |
| include(CheckIPOSupported) | |
| check_ipo_supported(RESULT IPO_SUPPORTED OUTPUT IPO_CHECK_MSG) | |
| if(IPO_SUPPORTED) | |
| message(STATUS "Link-time optimization enabled.") | |
| set(CMAKE_INTERPROCEDURAL_OPTIMIZATION ON) | |
| endif() | |
| else() | |
| if(CMAKE_BUILD_TYPE MATCHES "Release") | |
| include(CheckIPOSupported) | |
| check_ipo_supported(RESULT IPO_SUPPORTED OUTPUT IPO_CHECK_MSG) | |
| if(IPO_SUPPORTED) | |
| message(STATUS "Link-time optimization enabled.") | |
| set(CMAKE_INTERPROCEDURAL_OPTIMIZATION ON) | |
| endif() | |
| # Non-Release builds | |
| else() | |
| set(CMAKE_EXPORT_COMPILE_COMMANDS ON) | |
| endif() # CMAKE_BUILD_TYPE MATCHES "Release" |
🤖 Prompt for AI Agents
In components/core/CMakeLists.txt around lines 24 to 32, the if-else block
checking for Release build type is missing the closing endif() for the outer if
statement. To fix this, add an endif() after the else() branch content to
properly close the conditional block and prevent CMake configuration errors.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
LinZhihao-723
changed the title
3 participants
Description
From our microbenchmark result, enabling LTO (link-time optimization) can achieve up to 10% performance boost for clp-text and clp-json (with dependent libraries linked statically), tested on real user input. As its value has already been proven, this PR formally enables LTO through CMake for all core targets.
Benchmark Results
clp-text
The following benchmark was run on an Intel Core i9-14900K processor with 64GB of RAM, in Ubuntu 22.04 WSL, using GCC 11 for compilation. The affinity was set to core 0-3 (performance cores). The time is reported by the wall clock. Tested on an 18GB dataset (raw text) from our unstructured solution users.
Compression:
Without LTO: 108.8s
With LTO: 95.6s
Improvement: 13.8%
Search (with query
*original publishedPatch*):Without LTO: 28.2s
With LTO: 27.5s
Improvement: 2.5%
Decompression:
Without LTO: 51.5s
With LTO: 50.7s
Improvement: 1.6%
clp-json
The following benchmark was run on an Intel Core i7-9700K processor with 16GB of RAM, in Ubuntu 22.04, using GCC 11 for compilation. The time is reported by the wall clock. Tested on a 640MB dataset (kv-ir, 8.8GB before compression) from our structured solution users.
Compression:
Without LTO: 153.3s
With LTO: 150.9s
Improvement: 1.6%
Search (with query
*time*):Without LTO: 22.61s
With LTO: 21.72s
Improvement: 4.1%
Decompression:
Without LTO: 23.82s
With LTO: 23.37s
Improvement: 1.9%
Checklist
breaking change.
Validation performed
Summary by CodeRabbit