Skip to content

perf(image): link against musl and land on the static base - #329

Merged
BryanFRD merged 2 commits into
mainfrom
perf/musl-static-image
Sep 4, 2026
Merged

perf(image): link against musl and land on the static base#329
BryanFRD merged 2 commits into
mainfrom
perf/musl-static-image

Conversation

@BryanFRD

@BryanFRD BryanFRD commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Closes #327, whose first step landed in #328.

The binary is linked against musl and the runtime image is distroless/static instead of distroless/cc, which was carrying ten megabytes of glibc and libgcc for a binary that no longer needs either. Measured locally, both images built from this tree:

uncompressed
published ghcr.io/ferrlabs/lfsx:latest 13.5 MB
this branch, amd64 4.4 MB
this branch, arm64 4.2 MB

Built and smoke-tested locally on both architectures: /health answers ok, /ready answers 200, and with LFSX_AUTH=github a batch call reaches api.github.com and comes back 401 for a fake token, which is DNS and TLS working under musl rather than assumed to.

Cross-compiling

ring and zstd are C, so a musl build needs a C compiler for the target, and Debian packages no aarch64 musl toolchain. zig cc is one download that covers both architectures. Two things had to be pinned by trial rather than by guess:

  • zig 0.13 refuses --fix-cortex-a53-843419, which rustc passes for every aarch64 target, so the arm64 link failed outright. zig 0.14.1 with cargo-zigbuild 0.23.4 accepts it.
  • zig's tarball naming changed between 0.14.0 and 0.14.1, from zig-linux-x86_64- to zig-x86_64-linux-.

Two things found while doing it

The build step swallowed its own failures. That RUN chains with ; and never set -e, so when cargo build failed the layer still exported successfully with no binary in it, and the error surfaced three steps later as a confusing COPY --from=builder ... not found. It is how the first arm64 attempt reported itself. Left as is here, since the fix belongs in its own change, but worth knowing it is there.

The benchmark was measuring cargo. bench/throughput.sh launched the server with cargo run and then read VmRSS of $!, which is cargo's pid, not the server's: cargo stays alive as the parent. Every memory figure it has ever printed, including the ones quoted in docs/performance.md, was the resident size of the cargo wrapper, which is flat no matter what the server does. It now builds and launches the binary directly.

That fix is what makes the issue's acceptance condition possible, so it is here rather than in a follow-up: musl allocates differently under threads, and the whole point was to measure it rather than assume it. bench/throughput.sh takes an optional TARGET, and the benchmark workflow now runs both legs, host and x86_64-unknown-linux-musl, so the two numbers sit side by side. It also triggers on Dockerfile and Cargo.toml now, because those decide which build an operator actually runs.

The PR-time image check builds linux/arm64 as well, since the arm64 cross-compile is the half no laptop can try and the half that broke here.

What I have not measured

The throughput comparison itself. It needs a Linux runner, and it lands with this PR's own CI rather than from my machine. If musl's allocator costs anything at these sizes, that run is where it shows, and mimalloc is the answer if it does.

@BryanFRD
BryanFRD enabled auto-merge (squash) September 4, 2026 12:16
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

SonarQube — aucune nouvelle issue

Comparaison entre le projet bac à sable de cette PR et la branche par défaut : SonarQube Community n'analyse pas les PR, ce delta est calculé côté CI. Détail

@ferrfleet ferrfleet Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The musl switch itself reads correctly: --platform=$BUILDPLATFORM keeps the arm64 leg a real cross-compile rather than an emulated one, the final stage is COPY-only so distroless/static needs no emulation to assemble, and webpki-roots means dropping the system trust store costs nothing. The cargo run → direct binary fix in the benchmark is right, and the reason given for it is the correct one.

Blocking: docs/performance.md still publishes the numbers this PR proves were measuring cargo.

The table there says Resident memory, idle → peak | 5 MiB → 6 MiB, and the paragraph under it builds an argument on that row:

The memory row is the one worth looking at. A gigabyte moves through the process and its resident set grows by one megabyte, which is what "nothing is buffered" means in practice rather than as a claim.

By this PR's own account that is the resident size of the cargo wrapper, flat whatever the server did. Fixing the script and leaving the published figure in place keeps the false claim on the docs site, and the sentence "rather than as a claim" is exactly what it now is. The same reasoning you gave for landing the benchmark fix here rather than in a follow-up applies to the number it produced.

This PR's own bench run gives the replacement. Update the row and that paragraph from it, or strike the row and say it is being remeasured until CI has published a real one. Either is fine; leaving it is not. While in there, the intro also says the workflow "publishes a table on every change to the storage path", which is now two tables on a wider set of paths.

Two nits inline, neither withholding anything.

Comment thread bench/throughput.sh
Comment on lines +46 to 53
rustup target add "$TARGET" >/dev/null 2>&1 || true
cargo build --release --target "$TARGET" --bin lfsx-server >/dev/null 2>&1
server="target/${TARGET}/release/lfsx-server"
else
cargo build --release --bin lfsx-server >/dev/null 2>&1
server="target/release/lfsx-server"
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this is the same failure mode the PR body calls out in the Dockerfile. rustup target add ... || true eats the error, and cargo build ... 2>&1 >/dev/null eats the compiler's diagnostics, so under set -e a musl leg with no toolchain exits silently with no output at all. Redirecting only stdout keeps the noise down and leaves the reason visible.

Suggested change
rustup target add "$TARGET" >/dev/null 2>&1 || true
cargo build --release --target "$TARGET" --bin lfsx-server >/dev/null 2>&1
server="target/${TARGET}/release/lfsx-server"
else
cargo build --release --bin lfsx-server >/dev/null 2>&1
server="target/release/lfsx-server"
fi
if [ -n "$TARGET" ]; then
rustup target add "$TARGET" >/dev/null
cargo build --release --target "$TARGET" --bin lfsx-server >/dev/null
server="target/${TARGET}/release/lfsx-server"
else
cargo build --release --bin lfsx-server >/dev/null
server="target/release/lfsx-server"
fi

Comment thread Dockerfile
tar -xJf /tmp/zig.tar.xz -C /opt/zig --strip-components=1; \
ln -s /opt/zig/zig /usr/local/bin/zig; \
rm /tmp/zig.tar.xz; \
cargo install cargo-zigbuild --version "${ZIGBUILD_VERSION}" --locked

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this layer sits below COPY server ./server / COPY cli ./cli, so any source edit invalidates it and cargo install cargo-zigbuild --locked recompiles from source on the next build, once per platform, now that the verify job builds two. The zig tarball is re-downloaded with it. The sccache block above has the same placement, but it is a binary extract rather than a compile, so it is cheaper to ignore.

Moving both toolchain RUN blocks above the three COPY lines makes them depend only on the pinned ARG versions. Not something a suggestion block can express, since it also has to move lines outside this hunk.

Separately: the tarball is fetched with no integrity check. The version is pinned but the bytes are not, and ziglang.org publishes a shasum per artifact. Same gap as the sccache download, so it is pre-existing rather than introduced, but this is a compiler and a good place to close it.

@BryanFRD

BryanFRD commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Both handled.

The blocking one first. You are right that leaving those numbers up while the PR explains they measured cargo is the worse of the two states. The row is gone, and so is the paragraph that argued from it. What replaces it says plainly what happened and separates the two things that got tangled: the claim that nothing is buffered is not in doubt, the evidence for it was, and the number comes back when a run has produced it rather than being retyped from memory.

CI was failing for its own reason, unrelated to the review: sub-compilation of libunwind failed: CacheCheckFailed. zig keeps a compilation cache and refuses to work without a writable one, and under the rootless builder CI uses the default under $HOME was not. Pinned to /tmp/zig-cache through ZIG_GLOBAL_CACHE_DIR and ZIG_LOCAL_CACHE_DIR. It never showed locally because Docker Desktop runs that stage as root.

The same commit adds set -eu to that RUN, which is the swallowed-failure problem I flagged in the description. It is what turned the first arm64 failure into a misleading COPY --from=builder ... not found three steps later, and it would have hidden this one too if the linker error had not also broken the amd64 leg. Small enough to fix here rather than leave a known trap in a file this PR is already rewriting.

Rebuilt and re-smoke-tested locally after the change: 4.4 MB, /health answers.

@ferrfleet ferrfleet Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The blocking finding is fixed in c59532e: the memory row and the paragraph arguing from it are gone, and what replaces it separates the claim from the evidence instead of restating either. The remaining rows are client-side timings, unaffected by which pid was read, so leaving them stands up.

The rest of c59532e reads correctly. ZIG_GLOBAL_CACHE_DIR/ZIG_LOCAL_CACHE_DIR are set in the builder stage only, so nothing follows the binary into distroless/static. set -eu on that RUN does not change the sccache path: sccache --start-server ... && export RUSTC_WRAPPER=sccache is an AND-OR list, so a failing start is still ignored by -e and the build falls back to "not in use" rather than aborting. Checked in both bash and dash.

Note the image build was still running when I looked, so the arm64 leg under the new zig cache is unverified from here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Take the image from 15 MB to under 6

1 participant