Conversation
|
This pull request is stale because it has been open for 60 days with no activity. |
gdesmott
force-pushed
the
gd_10751_lto
branch
3 times, most recently
from
09dbf1c to
49a431a
Compare
Contributor
Automated performance analysis summaryThis comment was automatically generated given there is performance data available. In summary:
You can check a comparison in detail via the grafana link Performance Improvements - Comparison between master and gd_10751_lto.Time Period from 30 days ago. (environment used: oss-standalone)
Performance Regressions and Issues - Comparison between master and gd_10751_lto.Time Period from 30 days ago. (environment used: oss-standalone)
Tests with No Significant Changes (25 tests)Tests with No Significant Changes
|
gdesmott
force-pushed
the
gd_10751_lto
branch
6 times, most recently
from
370e9bf to
606b383
Compare
gdesmott
force-pushed
the
gd_10751_lto
branch
2 times, most recently
from
5e08f3d to
10d61c0
Compare
gdesmott
changed the title
gdesmott
force-pushed
the
gd_build_lto
branch
from
faa9473 to
9a1c5a8
Compare
gdesmott
force-pushed
the
gd_10751_lto
branch
from
75ab323 to
308d9ee
Compare
gdesmott
force-pushed
the
gd_build_lto
branch
from
9a1c5a8 to
37c76f3
Compare
gdesmott
force-pushed
the
gd_10751_lto
branch
4 times, most recently
from
e6ed0eb to
390f528
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
gdesmott
force-pushed
the
gd_10751_lto
branch
from
fde94ae to
4e80ee3
Compare
.github/workflows/task-test.yml
Outdated
Comment on lines
441
to
443
| LTO_PLATFORMS=("ubuntu:noble") | ||
| CONTAINER="${{ needs.get-config.outputs.container }}" | ||
| if [[ " ${LTO_PLATFORMS[@]} " =~ " ${CONTAINER} " ]]; then |
Collaborator
There was a problem hiding this comment.
Just want to confirm: is the intention to enable LTO in the merge queue? Because the current conditional won't enable it on PRs.
Collaborator
There was a problem hiding this comment.
(Since they don't run in containers)
Collaborator
Author
There was a problem hiding this comment.
Thanks I missed that.
I assumed the merge flow was a subset of the "Build on Platforms" one.
Adding a note about that then, we still need to update the build artifacts flow as well.
Collaborator
There was a problem hiding this comment.
Do we know how much longer the build takes when LTO is enabled in CI?
Collaborator
Author
There was a problem hiding this comment.
Does not seem to slow it down. From a quick unscientific test:
On x86_64:
- LTO: 45min 3s https://github.com/RediSearch/RediSearch/actions/runs/21139820826/job/60790770753
- no LTO: 45min 41s https://github.com/RediSearch/RediSearch/actions/runs/21138590696/job/60787573750
On aarch64:
- LTO: 1h 5m 45s https://github.com/RediSearch/RediSearch/actions/runs/21139820826/job/60790766224
- no LTO: 1h 7m 56s https://github.com/RediSearch/RediSearch/actions/runs/21138590696/job/60787817458
gdesmott
force-pushed
the
gd_10751_lto
branch
2 times, most recently
from
fbf2f8c to
03cd54c
Compare
This allow users to pick the right clang version manually by defining CC/CXX/LD without changing the default clang on their system.
Alpine's grep do not support -P so use sed instead.
Fix building tests with LTO on arm64.
Save us from re-checking if LTO is enabled or not.
Use the "append or define" pattern.
We should be good now that we call `make rust-tests LTO`
Linux distributions may ship either clang, clang-21 or both as binaries.
gdesmott
changed the title
gdesmott
force-pushed
the
gd_10751_lto
branch
from
eb40356 to
d2ea0bd
Compare
Collaborator
Author
|
@LukeMathWalker : as discussed, this PR now contains only |
LukeMathWalker
approved these changes
Jan 19, 2026
eyalrund
pushed a commit
that referenced
this pull request
Jan 22, 2026
* build: handle env variables when enabling LTO This allow users to pick the right clang version manually by defining CC/CXX/LD without changing the default clang on their system. * build: use new arg parsing syntax for LTO * build: fix LTO on Alpine Alpine's grep do not support -P so use sed instead. * build: enforce LLVM on Rust tests with LTO Fix building tests with LTO on arm64. * build: move up RUSTFLAGS lto Save us from re-checking if LTO is enabled or not. * build: simplify RUSTFLAGS declaration Use the "append or define" pattern. * build: no longer need to re-define RUST_TEST_COMMAND We should be good now that we call `make rust-tests LTO` * build: try using llvm versioned binaries if available Linux distributions may ship either clang, clang-21 or both as binaries.
LukeMathWalker
pushed a commit
that referenced
this pull request
Jan 26, 2026
* build: handle env variables when enabling LTO This allow users to pick the right clang version manually by defining CC/CXX/LD without changing the default clang on their system. * build: use new arg parsing syntax for LTO * build: fix LTO on Alpine Alpine's grep do not support -P so use sed instead. * build: enforce LLVM on Rust tests with LTO Fix building tests with LTO on arm64. * build: move up RUSTFLAGS lto Save us from re-checking if LTO is enabled or not. * build: simplify RUSTFLAGS declaration Use the "append or define" pattern. * build: no longer need to re-define RUST_TEST_COMMAND We should be good now that we call `make rust-tests LTO` * build: try using llvm versioned binaries if available Linux distributions may ship either clang, clang-21 or both as binaries.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Various improvements to
build.shneeded to integrate LTO into the CI.Mark if applicable
Note
Improves cross-language LTO handling and robustness in
build.sh.clang/clang++/lldvia$CC/$CXX/$LD, falling back to versionedclang-$RUSTC_LLVM_VERSION/lld-$RUSTC_LLVM_VERSION, else unversioned toolsCMAKE_C_COMPILER,CMAKE_CXX_COMPILER,-fuse-ld) and Rust (RUSTFLAGSwithlinker-plugin-lto,-C linker=...,-C link-arg=-fuse-ld=...)LTO?(=1)) andRUSTFLAGScomposition (compactRUST_DYN_CRThandling); removes redundant LTO-specificRUSTFLAGSexport blockWritten by Cursor Bugbot for commit d2ea0bd. This will update automatically on new commits. Configure here.