perf(docker): enable -Zshare-generics in the image build - #845
Conversation
rustc instantiates generics and async state machines in every crate that uses them, and the per-crate symbols defeat LTO merging; share-generics makes downstream crates reuse upstream monomorphizations instead. Measured on the release bin: .text 11.65 MiB to 11.00 MiB (-666 KiB, -5.6%), cross-crate duplicate-symbol waste 1414 KiB to 475 KiB, consumer-crate reinstantiation 1484 KiB to 531 KiB. Full wacore + lib suites pass built with the flag (1838 tests). Nightly-only, which the image already pins through rust-toolchain.toml; the stable CI lane is untouched since the flag lives only here. With fat LTO the historical cross-crate inlining downside does not apply (LTO sees all bitcode), and CodSpeed cannot exercise a Dockerfile-only flag, so the test run above is the functional validation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c2fcd6016
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
The cook already ran on the pinned nightly (the chef stage's copy sits at / and rustup walks parent directories), but with nightly-only RUSTFLAGS in play that subtlety becomes load-bearing; an explicit copy in /app keeps the build independent of the base image's default WORKDIR.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDockerfile updates: chef stage now installs ChangesDocker build updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 27s —— View job Claude PR Review
SummaryLGTM. The change is small, well-targeted, and the Codex P1 was addressed correctly by the follow-up commit Codex P1 — resolvedCodex correctly pointed out that on the original commit, Worth noting: rustup walks up from cwd, so the override at Correctness checks
Minor observations (not blocking)
VerdictApproving the approach. The Codex P1 is resolved by the follow-up commit, the flag's safety story checks out against • branch: |
![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
🤖 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 `@Dockerfile`:
- Line 33: The Dockerfile currently sets RUSTFLAGS to include "-C
target-cpu=native -Zshare-generics=y", which bakes the build host CPU features
into the binary; remove the "-C target-cpu=native" token from the ENV RUSTFLAGS
assignment (keep "-Zshare-generics=y") or replace it with a portable baseline
CPU flag (e.g., a conservatively chosen target) so the builder-stage ENV
RUSTFLAGS line no longer forces native CPU instructions and the produced
artifact is portable to older production CPUs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Pins the tool's own dependency graph to its published lockfile so an image rebuild cannot break on a freshly released or yanked transitive dep.
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 `@Dockerfile`:
- Line 18: The RUN invocation installing cargo-chef should pin the cargo-chef
release to ensure deterministic rebuilds; update the RUN line that currently
says `cargo install cargo-chef --locked` to include an explicit `--version
<exact-version>` (e.g., `--version 0.x.y`) alongside `--locked` so the install
uses a fixed cargo-chef release rather than the latest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
The prebuilt rustup std does not participate in fat LTO, so its code ships as-is. Building it from rust-src with the image's release profile lets LTO inline and dead-code-eliminate it like any other crate: another -303 KiB of .text on top of share-generics, with the full wacore + lib suites passing built this way (1838 tests). The env form (CARGO_UNSTABLE_BUILD_STD) reaches both the chef cook and the final build so the dependency cache layer stays valid; build-std requires an explicit target, hence the musl triple on both invocations and the adjusted binary path in the runtime stage.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
--locked freezes the tool's dependency graph but not which release gets fetched; the explicit --version makes image rebuilds deterministic.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Dockerfile (2)
18-19: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPin
cargo-chefto an exact version for deterministic builds.Look,
--lockedonly freezes cargo-chef's own dependency graph—it doesn't stopcargo installfrom grabbing whatever the latest cargo-chef release happens to be. For a production image we ship to billions of users, we need reproducibility. Add--version <exact>so CI builds are byte-for-byte consistent.-RUN rustup show && rustup component add rust-src && cargo install cargo-chef --locked +RUN rustup show && rustup component add rust-src && cargo install cargo-chef --locked --version 0.1.71(Replace
0.1.71with the current stable cargo-chef release.)🤖 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 `@Dockerfile` around lines 18 - 19, Pin the cargo-chef installation to an exact release so builds are deterministic: update the RUN that installs cargo-chef (the line calling "cargo install cargo-chef --locked") to include "--version 0.1.71" (or the current stable release) in addition to "--locked" so cargo-install cannot pull a newer release; leave the rustup component add rust-src call unchanged.
34-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop
target-cpu=native—this is a portability time bomb.This is critical. We're baking whatever CPU features my build machine has into the binary, then shipping it in a
scratchimage that could run anywhere. On a production server with an older CPU, users getSIGILLand a dead container. That's not the kind of user experience I'm trying to deliver here.Keep
-Zshare-generics=yfor the codegen wins, but removetarget-cpu=nativeor replace it with a safe baseline likex86-64-v2if you need some SIMD.-ENV RUSTFLAGS="-C target-cpu=native -Zshare-generics=y" +ENV RUSTFLAGS="-Zshare-generics=y"🤖 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 `@Dockerfile` at line 34, The Dockerfile sets ENV RUSTFLAGS="-C target-cpu=native -Zshare-generics=y", which bakes host CPU features into the binary and can cause SIGILL on older machines; remove the "-C target-cpu=native" token (or replace it with a portable baseline like "-C target-cpu=x86-64-v2" if you need SIMD) and keep "-Zshare-generics=y" for codegen benefits, i.e., edit the ENV RUSTFLAGS line to omit target-cpu=native and ensure only the desired, portable flags remain.
🤖 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.
Duplicate comments:
In `@Dockerfile`:
- Around line 18-19: Pin the cargo-chef installation to an exact release so
builds are deterministic: update the RUN that installs cargo-chef (the line
calling "cargo install cargo-chef --locked") to include "--version 0.1.71" (or
the current stable release) in addition to "--locked" so cargo-install cannot
pull a newer release; leave the rustup component add rust-src call unchanged.
- Line 34: The Dockerfile sets ENV RUSTFLAGS="-C target-cpu=native
-Zshare-generics=y", which bakes host CPU features into the binary and can cause
SIGILL on older machines; remove the "-C target-cpu=native" token (or replace it
with a portable baseline like "-C target-cpu=x86-64-v2" if you need SIMD) and
keep "-Zshare-generics=y" for codegen benefits, i.e., edit the ENV RUSTFLAGS
line to omit target-cpu=native and ensure only the desired, portable flags
remain.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14897712ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
build-std needs an explicit --target, but hard-coding x86_64 broke multi-arch image builds that previously used the base image's native target. Reading the triple from rustc keeps native semantics on every platform; the binary is copied to a fixed path so the scratch stage stays arch-agnostic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Dockerfile (1)
36-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t ship host-CPU-specific instructions in a portable scratch image.
Line 36 still bakes build-host CPU features into the release binary. That can crash on less-capable production CPUs (
Illegal instruction). Keep-Zshare-generics=y, droptarget-cpu=nativeor replace it with an explicit portable baseline.Suggested minimal fix
-ENV RUSTFLAGS="-C target-cpu=native -Zshare-generics=y" +ENV RUSTFLAGS="-Zshare-generics=y"🤖 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 `@Dockerfile` at line 36, The Dockerfile currently sets ENV RUSTFLAGS including "-C target-cpu=native" which bakes host-specific CPU instructions into the release binary; remove the "-C target-cpu=native" token (or replace it with an explicit portable baseline like a generic x86_64 target flag) and keep the "-Zshare-generics=y" option so portability is preserved; update the ENV RUSTFLAGS assignment (the single line containing ENV RUSTFLAGS) to exclude the native target and verify the resulting build artifacts run on older CPUs.
🤖 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 `@Dockerfile`:
- Line 53: The RUN line that extracts the Rust host triple (currently using
`rustc -vV | sed -n 's/^host: //p' > /rust-target`) should be made fail-fast:
capture the output into an intermediate file (e.g., `/tmp/rust-host`) and then
verify it is non-empty before moving to `/rust-target`; if empty, exit with a
non-zero status and print a helpful error. Update the Dockerfile to run `rustc
-vV` into the temp file, strip the `host:` field with `sed` into the same temp,
assert the file has content (or `test -s` / `[[ -s ]]`) and only then `mv` to
`/rust-target`, so any failure is detected immediately during image build.
---
Duplicate comments:
In `@Dockerfile`:
- Line 36: The Dockerfile currently sets ENV RUSTFLAGS including "-C
target-cpu=native" which bakes host-specific CPU instructions into the release
binary; remove the "-C target-cpu=native" token (or replace it with an explicit
portable baseline like a generic x86_64 target flag) and keep the
"-Zshare-generics=y" option so portability is preserved; update the ENV
RUSTFLAGS assignment (the single line containing ENV RUSTFLAGS) to exclude the
native target and verify the resulting build artifacts run on older CPUs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Docker's default shell has no pipefail, so a failing rustc would be masked by the pipe and surface later as a cryptic empty --target; test -s trips at the right layer.
1 participant
What
Adds
-Zshare-generics=yto the Docker image'sRUSTFLAGS.Why
The cargo-bloat audit showed that rustc instantiates generics and async state machines in every crate that uses them; the per-crate symbol copies carry distinct instantiating-crate hashes that even fat LTO cannot merge. #842/#843 removed the biggest sources at the library level, but the long tail (moka cache machinery, async send graphs reached from handler closures, drop glue) is inherent to how stable rustc codegens cross-crate.
share-genericsis the upstream switch for exactly this: downstream crates reuse upstream monomorphizations instead of re-codegening them.Measured (release bin, current main)
.text: 11.65 MiB -> 11.00 MiB (-666 KiB, -5.6%)Cumulative with #842/#843/#844 the audit took
.textfrom 13.03 MiB to 11.00 MiB (-15.6%).Safety
rust-toolchain.toml. The stable CI lane and stable consumers are untouched: the flag lives only in the Dockerfile.lto = "fat"that does not apply, since LTO sees all bitcode and re-inlines freely.Note for downstream images (e.g. Veloz): the same one-line flag applies to any consumer building on the pinned nightly.