feat: cross-language LTO to inline C TLS shim into Rust FFI#1982
feat: cross-language LTO to inline C TLS shim into Rust FFI#1982yannham wants to merge 11 commits into
Conversation
📚 Documentation Check Results📦
|
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
🔒 Cargo Deny Results📦
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1982 +/- ##
==========================================
+ Coverage 72.68% 72.88% +0.19%
==========================================
Files 451 458 +7
Lines 74255 75783 +1528
==========================================
+ Hits 53971 55233 +1262
- Misses 20284 20550 +266
🚀 New features to boost your workflow:
|
|
Add an opt-in inline mode via the LIBDD_OTEL_THREAD_CTX_INLINE env var that uses cross-language LTO (clang + lld) to inline the C TLS shim directly into the Rust FFI functions, eliminating a function-call indirection on every TLS access. When the env var is set, build.rs validates that clang and a suitable LLD are available, compiles the C shim as LLVM bitcode (-flto=thin), and emits the linker flags needed for cross-language LTO. A companion build-optimized.sh wrapper sets the target-scoped RUSTFLAGS and env var. When the env var is absent, the previous behavior is preserved: the default cc compiles the shim with -mtls-dialect=gnu2 on x86-64, no LTO, no inlining. Also fixes #[cfg(target_os/arch)] in build scripts to use the correct CARGO_CFG_* env vars for cross-compilation correctness. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
After a successful build, verify with nm that the C TLS shim symbol was actually inlined away. Warns and exits 1 if inlining failed, or warns (but succeeds) if nm is not available. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
yannham
force-pushed
the
yannham/thread-ctx-inlining-exp
branch
from
99d5622 to
88d7f6d
Compare
yannham
force-pushed
the
yannham/thread-ctx-inlining-exp
branch
2 times, most recently
from
2a54297 to
9932cac
Compare
Deduplicate the function that locates the toolchain's bundled rust-lld by moving it into build_common, where both build scripts can reuse it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
yannham
force-pushed
the
yannham/thread-ctx-inlining-exp
branch
from
9932cac to
9227252
Compare
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
| let target_os = env::var("CARGO_CFG_TARGET_OS").unwrap(); | ||
| let target_arch = env::var("CARGO_CFG_TARGET_ARCH").unwrap(); | ||
|
|
||
| if target_os != "linux" { |
There was a problem hiding this comment.
This change is somehow unrelated, but using static cfg(target_os = "linux") is actually not the way to go in a build script (during cross-compilation typically, the target_os of the script and of the dynamic library could differ), so fixing it passing by.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
I really like this optimization, left a few comments since I'm a bit... confused about some of the details :D
There was a problem hiding this comment.
So... I'm curious (and this is not a blocker for the PR) -- does this script need to exist?
That is, do the environment variables need to be defined before cargo build gets invoked, or is there perhaps a path to making this something that we could have as a flag in our builder crate?
I'm thinking that if we could move stuff to rust code, it would probably be a bit more ergonomic for users + easier to maintain and validate.
There was a problem hiding this comment.
Unfortunately, I think we need the script (or something similar that sets the flags from outside of the build) and can't be fully automatic. This is indeed annoying. Quoting this PR's description:
A wrapper script is needed because build.rs cannot set rustc codegen flags (-Clinker-plugin-lto, -Clinker=clang). Those must come from RUSTFLAGS.
My understanding is that by design (for reasons I'm not sure to understand, to be honest), the build.rs can't set compiler flags, only linker or linking-related flags. Cf https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-flags (and found other people wanting to do the same, and couldn't).
There was a problem hiding this comment.
I was wondering this too. I put a comment further down that we might be able to do this with .cargo/config.toml, but that would be project wide if it did work
There was a problem hiding this comment.
I was wondering this too. I put a comment further down that we might be able to do this with .cargo/config.toml, but that would be project wide if it did work
Good point, this is why I didn't do it in libdatadog, but if you're having a libdatadog-mylang facade crate anyway, that might be doable option? Will add this to the readme.
There was a problem hiding this comment.
Oh, I missed that bit in the PR description, although I'd say that it very much deserves to live in a why does this script need to exist in the script itself :)
Having said that, I understand the limitation around build.rs (thanks for clarifying), yet on our own setup we often use the builder crate that then invokes cargo for us; could we auto-add the flags when libdatadog is built in that manner perhaps?
There was a problem hiding this comment.
I'm not familiar with the builder crate, but all it takes is to set two env variable before cargo starts (RUST_FLAGS and the toggle introduced here). So it sounds like we could definitely do that 👍
There was a problem hiding this comment.
Afaik right now the builder crate is the recommended way ™️ to build libdatadog if you're not consuming it from rust.
There was a problem hiding this comment.
(Although supporting it is not a blocker for this PR, to be clear, although it is a really nice to have ™️)
| SO="$REPO_ROOT/target/$TARGET/release/liblibdd_otel_thread_ctx_ffi.so" | ||
|
|
||
| if [[ -f "$SO" ]] && nm "$SO" 2>/dev/null | grep -q 'libdd_get_otel_thread_ctx'; then | ||
| echo >&2 "WARNING: build succeeded but the C TLS shim (libdd_get_otel_thread_ctx_v1) was NOT inlined." |
There was a problem hiding this comment.
Very minor: Since we have the exit 1 at the bottom, this is not really a warning ;)
(And I think having the exit 1 is correct btw!)
| ./build-optimized.sh --target aarch64-unknown-linux-gnu | ||
| ``` | ||
|
|
||
| Extra arguments are forwarded to `cargo build`. |
There was a problem hiding this comment.
It might be useful to document a way for downstreams to validate that the optimized build did happen. E.g. I'd love to be able to add a check in https://github.com/DataDog/libdatadog-rb/blob/main/spec/gem_packaging.rb to validate that the LTO was correctly in use.
There was a problem hiding this comment.
This is already automatically verified in the script, but I can surely reproduce the steps here in the readme, if you think that's useful.
There was a problem hiding this comment.
My thinking is -- the script checks, but who's checking that the script is in use? ;)
Maybe someone redid the ruby build build entirely and forgot to even add the script -- that's the thing I was thinking of catching.
scottgerring
left a comment
scottgerring
left a comment
There was a problem hiding this comment.
Nice - great to see the improvements to :D
| REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" | ||
| SO="$REPO_ROOT/target/$TARGET/release/liblibdd_otel_thread_ctx_ffi.so" | ||
|
|
||
| if [[ -f "$SO" ]] && nm "$SO" 2>/dev/null | grep -q 'libdd_get_otel_thread_ctx'; then |
There was a problem hiding this comment.
nit: if nm fails it looks like the symbol was absent and stderr is suppressed; maybe we could do something like TODO?
There was a problem hiding this comment.
annnnd I was meant to replace that TODO with a concrete suggestion
| # CARGO_TARGET_<TRIPLE>_RUSTFLAGS scopes the flags to the target only, keeping | ||
| # build scripts and proc-macros unaffected. | ||
| TARGET_ENV=$(echo "$TARGET" | tr 'a-z-' 'A-Z_') | ||
| export "CARGO_TARGET_${TARGET_ENV}_RUSTFLAGS=-Clinker-plugin-lto -Clinker=clang" |
There was a problem hiding this comment.
Should we try preserve existing CARGO_TARGET_${TARGET_ENV}_RUSTFLAGS and append to it? I don't have enough surrounding context on our build infra landscape but blasting over it in its entirety might be unexpected.
| -p libdd-otel-thread-ctx-ffi \ | ||
| "${EXTRA_ARGS[@]}" | ||
|
|
||
| # Sanity-check that the C shim was actually inlined, if `nm` is available. |
There was a problem hiding this comment.
If
nmwas available
Will this be the case in our CI build pipeline for the repo?
There was a problem hiding this comment.
No, and this is the point: I don't want to fight against CentOS and Alpine with the requirements for LTO (clang, lld, LLVM 18+, ...). I think this will be on the build workflow of the final dynamic library to check that.
|
|
||
| build.file("src/tls_shim.c").compile("tls_shim"); | ||
| println!("cargo:rerun-if-changed=src/tls_shim.c"); | ||
| if target_arch == "x86_64" { |
There was a problem hiding this comment.
This isn't really related to this PR, but should we throw if we see Linux with a different architecture from these two? That way in the future if some enterprising engineer adds risc-v or whatever, we'll know that we have to address this.
I figure the linker flags to specify dialect differ between architectures anyway, so it is unlikely this will just work out of the box.
| @@ -0,0 +1,63 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
I haven’t looked deeply into this and it may be a rabbit hole, but could .cargo/config.toml help with the required codegen flags here? My understanding is that it would likely need to be workspace/repo-wide which broadens the impact compared with this wrapper script. But maybe longer-term we want this kind of LTO setup in the release pipeline anyway?
There was a problem hiding this comment.
Yes, indeed .cargo/config.toml is an alternative way. However the blast radius is much bigger: as you say, this is repo-wide, and additionally it can't be triggered "dynamically" for different build modes. Typically I fear that might be hell for the CI, and even on downstream SDKs, some aren't necessarily using latest clang versions (I remember the first otel thread context PR broke PHP because they weren't using a recent enough clang). But it's an alternative, and it should be documented 👍
There was a problem hiding this comment.
Might be worth smoking it out on the CI for this repo just to see what breaks? Adds weight to the "here's why we have a separate script" thing 🤷 don't feel super strongly about this
There was a problem hiding this comment.
The CI is annoying but can probably get sorted? The thing I'm more worried about with this is downstream users like dd-trace-php. I can try on the CI, but I think it's insufficient. We would have to make sure it doesn't break any downstream consumer of libdatadog, all the dd-trace-xxx. Even worse, with dd-trace-rs, we impose constraints on the toolchain of end-users/customers directly by transitivity. We can discuss it in the component team but my gut feeling is that it's a a bold move to force everyone to use our specific configuration and clang.
There was a problem hiding this comment.
my gut feeling is that it's a a bold move to force everyone to use our specific configuration and clang.
Yeah, I'd +1 that as well. Ideally we can make it as turn-key and easy to do and hard to miss as possible, but not go as far as making it a hard requirement.
| /// Returns the rust-lld `gcc-ld/` directory if found; `None` means the system | ||
| /// `ld.lld` will be used instead. Panics with a clear message when the | ||
| /// requirements are not met. | ||
| fn require_lld_for_inline(target_arch: &str) -> Option<PathBuf> { |
There was a problem hiding this comment.
The name reads a little funny to me in terms of what the function does; maybe resolve works better?
| fn require_lld_for_inline(target_arch: &str) -> Option<PathBuf> { | |
| fn resolve_lld_for_inline(target_arch: &str) -> Option<PathBuf> { |
Co-authored-by: Scott Gerring <scottgerring@users.noreply.github.com>
4 participants
What does this PR do?
Add opt-in cross-language LTO so the C TLS shim (
libdd_get_otel_thread_ctx_v1) is inlined directly into the Rust FFI functions, eliminating a function-call indirection on every TLS access.The mode is controlled by the
LIBDD_OTEL_THREAD_CTX_INLINEenv var. Abuild-optimized.shwrapper script sets the env var and the target-scopedRUSTFLAGSneeded for cross-language LTO. Without the env var, the build behaves exactly as before.Motivation
The OTel thread-level context spec requires TLSDESC for the TLS variable, which forces us through a C shim since stable
rustcdoesn't expose TLS dialect control. This adds a function call on everyattach/detach. Cross-language LTO eliminates that call.Benchmark results (median of 3 batch runs on x86-64):
Additional Notes
clangandlld(the toolchain's bundledrust-lldis used automatically).build.rscannot set rustc codegen flags (-Clinker-plugin-lto,-Clinker=clang). Those must come fromRUSTFLAGS.#[cfg(target_os/arch)]in build scripts are replaced with the correctCARGO_CFG_*env vars for cross-compilation correctness.How to test the change?