Ship the Rust core with fat LTO and stripped symbols - #343
Conversation
Whole-program LTO plus strip cuts the cdylib ~15% (1.51 -> 1.29 MB); the native scatter bench shows no runtime change beyond +/-15% run-to-run noise. .dynsym survives stripping, so ctypes binding is unaffected (abi_smoke passes 140/140 against the stripped build). panic stays unwinding because lib.rs catches panics at the C ABI; the wasm target keeps its explicit -C panic=abort in release.yml. Profile rationale and the not-adopted PGO lever are recorded in spec/design/rust-engine.md.
📝 WalkthroughWalkthroughThe Rust release profile now uses fat LTO, one codegen unit, and symbol stripping. Design documentation and the changelog record the panic-unwinding constraints and the resulting ChangesRust release profile optimization
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 73-76: Update the Rust core release-profile changelog entry to
document all three settings, including codegen-units = 1, and narrow the
stripping description to state that symbol tables are stripped while preserving
.dynsym. Match the terminology and rationale used in spec/design/rust-engine.md,
while retaining the size and benchmark results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da24bbcc-af73-4a2c-875e-1170755eafcf
📒 Files selected for processing (3)
CHANGELOG.mdCargo.tomlspec/design/rust-engine.md
| - The Rust core's release profile now uses fat LTO and strips symbol tables, | ||
| shrinking the shipped cdylib ~15% (1.51 → 1.29 MB) with no measured runtime | ||
| change on the native scatter bench (`spec/design/rust-engine.md` §2 records | ||
| the profile and why `panic` stays unwinding). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document all release-profile changes and narrow the stripping claim.
The implementation changes three profile settings, but this entry omits codegen-units = 1. “Strips symbol tables” is also broader than the specification, which says .dynsym is preserved. Please align the wording with spec/design/rust-engine.md.
Proposed wording
-- The Rust core's release profile now uses fat LTO and strips symbol tables,
+- The Rust core's release profile now uses fat LTO, one codegen unit, and
+ strips `.symtab`/debug information while preserving dynamic exports,
shrinking the shipped cdylib ~15% (1.51 → 1.29 MB) with no measured runtime📝 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.
| - The Rust core's release profile now uses fat LTO and strips symbol tables, | |
| shrinking the shipped cdylib ~15% (1.51 → 1.29 MB) with no measured runtime | |
| change on the native scatter bench (`spec/design/rust-engine.md` §2 records | |
| the profile and why `panic` stays unwinding). | |
| - The Rust core's release profile now uses fat LTO, one codegen unit, and | |
| strips `.symtab`/debug information while preserving dynamic exports, | |
| shrinking the shipped cdylib ~15% (1.51 → 1.29 MB) with no measured runtime | |
| change on the native scatter bench (`spec/design/rust-engine.md` §2 records | |
| the profile and why `panic` stays unwinding). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 73 - 76, Update the Rust core release-profile
changelog entry to document all three settings, including codegen-units = 1, and
narrow the stripping description to state that symbol tables are stripped while
preserving .dynsym. Match the terminology and rationale used in
spec/design/rust-engine.md, while retaining the size and benchmark results.
Merging this PR will not alter performance
Comparing Footnotes
|
What
Switches the release profile from thin to fat LTO and strips symbol tables from the shipped cdylib:
Measured locally (Linux x86_64)
scripts/bench_scatter_native.py— 10M-point density prep reads 7.2–8.5 ms across repeats on both builds; run-to-run noise (±15%) dominates. CodSpeed on this PR is the cleaner verdict.scripts/abi_smoke.pypasses 140/140 against the stripped build —stripremoves.symtab/debuginfo only, and.dynsym(what ctypes binds against) survives.Deliberately not changed
panicstaysunwind: the C-ABI backstop inlib.rsconverts kernel panics into sentinel returns viacatch_unwind;panic = "abort"would abort the embedding CPython process. The wasm target keeps its explicit-C panic=abortinrelease.yml, where unwinding is unsupported anyway.target-cpubump: wheels stay baseline x86-64 (manylinux_2_17); AVX2 is already runtime-dispatched insrc/simd.rs.spec/design/rust-engine.md§2 as an open lever needing per-target training workloads in the release matrix.Spec updated (
spec/design/rust-engine.md§2) with the profile, the measurements, and the panic constraint; changelog entry under Unreleased.Summary by CodeRabbit
Performance
Documentation