Optimize release profile

[simonmarty]

Issue #, if available: See #9

https://doc.rust-lang.org/cargo/reference/profiles.html

Description of changes:

Experimenting with release profile parameters

• lto = true to use link time optimization across the dependency tree.
• codegen-units = 1, significantly increases compilation time, is supposed to help with optimization.
• strip = true Removes symbols and debug info.

Drops the x86_64-unknown-linux-gnu binary size from 29MB to 14MB.

Before merging:

Run a load test and verify this leads to a consistent performance improvement on

• stable-x86_64-unknown-linux-gnu
• stable-aarch64-unknown-linux-gnu
• stable-x86_64-pc-windows-msvc

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.35%. Comparing base (6bf8c81) to head (05b2a4e).

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #38   +/-   ##
=======================================
  Coverage   86.35%   86.35%           
=======================================
  Files          34       34           
  Lines        9422     9422           
  Branches     9422     9422           
=======================================
  Hits         8136     8136           
  Misses       1153     1153           
  Partials      133      133           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[secrets-manager-ai-code-review]

Claude finished @simonmarty's task in 52m 45s —— View job

---

Code review: Optimize release profile

• Gather context (workflows, Dockerfile, manifests, branch history)
• Run /code-review on the diff
• Post inline comments for specific issues — 11 posted
• Verify the load-bearing claims against this PR's own CI logs
• Post review summary

Three knobs, one number, and the number doesn't describe this diff. Details below; 11 inline comments are on the changed lines.

---

Two things I verified that bear directly on the unchecked checklist

1. The 29MB → 14MB figure was measured against lto = "thin", on a binary that no longer exists.

git show b37d027 -- Cargo.toml (Nov 18 2024) added this block with lto = "thin", when members = ["aws_secretsmanager_agent", "aws_secretsmanager_caching"]. The head commit 05b2a4e (today) is a one-line escalation:

-lto = "thin"
+lto = true          # `true` == "fat", not thin

So the headline number describes thin LTO on aws_secretsmanager_agent in a two-member workspace. What's being merged is fat LTO on a six-member workspace producing aws-workload-credentials-provider v3.1.1. The three "Before merging" boxes can't be closed against that number — and if reviewers take 14MB at face value, the setting they actually approved is lto = "thin".

2. cargo bench silently inherits this profile, and the cost is measurable in this PR's own runs.

Cargo's bench profile inherits release. Confirmed empirically in run 33004416135:

19:27:35  Run cargo bench --bench benchmark -- --output-format bencher | tee ../bench-result.txt
19:31:09  Finished `bench` profile [optimized] target(s) in 3m 33s
19:31:20  test CacheHit ... bench:            222 ns/iter (+/- 13)
19:31:32  test CacheEviction ... bench:    37,489 ns/iter (+/- 2,618)

3m33s of build for ~23s of measurement, on a job with no cargo/target cache (its only actions/cache holds benchmark-data.json). And in Docker CI run 33004416271, the fat-LTO tail is a single serialized compilation unit:

#12 314.3    Compiling aws_workload_credentials_provider v3.1.1
#12 456.3     Finished `release` profile [optimized] target(s) in 7m 36s
#12 DONE 456.6s

142s in one unit — 31% of the whole build. The arm64 leg shows ~139.5s (41.7% of its build), i.e. essentially identical, which is what you'd expect from a single-threaded whole-program LLVM pass: faster cores buy nothing. docker.yml runs this on both arches for every PR with no cache-from/cache-to, and no job in .github/workflows/ sets timeout-minutes (grep: zero matches), so the guard rail is GitHub's 360-minute default.

---

Highest-value findings (all inline)

Debuggability — the size win doesn't require this. strip = true is strip = "symbols". Release already defaults to debug = 0, so on ELF its only effect is deleting .symtab — precisely what RUST_BACKTRACE and addr2line/perf need — from a long-running credentials daemon whose release pipeline archives no symbol artifact at all (staging.yml:115-121 and release.yml:101-106 move only the bare binary). strip = "debuginfo" keeps nearly the whole size win. Fix this →

On x86_64-pc-windows-msvc — one of the three shipped targets, and an unchecked box — MSVC keeps debug info in a side-car PDB, so there's ~no size win to collect, while rustc's MSVC path implements -Cstrip by dropping /DEBUG. Worth one command before merging: cargo build --release --target x86_64-pc-windows-msvc then check whether a .pdb still appears, with and without the diff. The Windows build ships as a Service, where WER minidumps are the field diagnostic.

The perf guard gets blinded, not tripped. benchmarks.yml compares against cached history with alert-threshold: '300%' / fail-on-alert: true. Post-merge, LTO'd numbers meet a pre-LTO baseline; since the threshold only fires on regressions, a large apparent speedup is silently accepted and becomes the permanent baseline. Compounding it, neither iter closure in benchmark.rs returns a value (both end .unwrap();), so criterion's internal black_box protects nothing — and fat LTO is exactly what makes the previously-opaque cross-crate get_secret_value inlinable with a provably unused result. Fix this →

Altitude. Only two commands in the repo build the shipped artifact (staging.yml:91, :98). Overriding shared release also hits Dockerfile:10, cargo bench, and the five documented cargo build --release invocations in the README — every contributor and customer pays a fat-LTO link for a size win only the S3 artifact needs. [profile.dist] inherits = "release" confines it; cost is 5 path updates in staging.yml (91, 98, 104, 119, 137). Fix this →

codegen-units = 1 is largely subsumed by fat LTO. Fat LTO already merges everything into one module, so the pre-LTO partitioning this controls is erased by the merge; what survives is serialized codegen. It's the one line with an admitted cost and no measured benefit.

Unnecessary work in the release build, now much more expensive. The root manifest sets no default-members, so cargo build --release builds integration-tests. Visible in the same log — #12 215.3 Compiling reqwest v0.13.4, #12 290.7 Compiling integration-tests v0.1.0 — none of which reaches the shipped binary. Fix this →

Nothing validates the result. No job in the repo ever executes a release-profile binary — rust.yml is dev-profile throughout, and the shipped artifact's only check is the readelf glibc guard. That guard also fails open: if required is empty, highest is empty, printf '%s\n%s\n' "" "2.26" | sort -V | tail -n1 yields 2.26, the comparison equals $AL2_GLIBC_VERSION, and it reports success. strip = true does not break it today (.dynsym/.gnu.version_r are SHF_ALLOC and survive --strip-all), but this diff is the first change to how that binary carries symbols. Fix this →

Also inline: the documented --features fips release build (aws-lc FIPS module, runtime integrity self-check) is compiled by no workflow in release; and no automated guard exists for the two targets the checklist admits are unverified — staging.yml:100-113 already runs per-target post-build verification and is the natural home for a binary-size budget.

---

Suggested shape

strip alone plausibly accounts for most of the 14MB. If that holds, the cheapest version keeping the demonstrated benefit is one line. Concretely, I'd suggest: measure the three knobs separately on today's tree; use strip = "debuginfo" unless you also archive symbols; put whatever survives behind [profile.dist] so cargo bench, the per-PR Docker builds, and README readers stay fast; and pin a size budget next to the existing glibc guard so the win can't silently regress.

Not done: cargo wasn't runnable in this sandbox, so per-knob size attribution and the MSVC PDB behavior rest on documented rustc/Cargo semantics rather than measurement — flagged as such above. Everything I stated as measured comes from this PR's own CI logs or git.
· release-profile

[secrets-manager-ai-code-review]

strip = true destroys the symbol table for every release build, and nothing archives symbols anywhere.

In Cargo, strip = true is strip = "symbols" — it removes the DWARF and .symtab. The shipped artifact is built at .github/workflows/staging.yml:91 / :98, uploaded at staging.yml:115-121, and re-uploaded to S3 by release.yml:101-105. Only the bare binary moves through that pipeline: no .debug file, no .pdb, no dSYM. There is also no custom panic hook anywhere in the workspace (no set_hook / backtrace matches), so panics use the default hook.

Failure scenario: a customer's long-running aws-workload-credentials-provider-sm.service panics in the field. RUST_BACKTRACE=1 prints bare hex addresses, and because no symbol artifact was retained you cannot symbolize it after the fact — the crash is undiagnosable.

strip = "debuginfo" gets essentially the whole 29MB→14MB win (the bulk is std's DWARF, since debug = 0 already applies in release) while keeping .symtab so backtraces stay symbolized:

Suggested change

	strip = true
	strip = "debuginfo"

[secrets-manager-ai-code-review]

On x86_64-pc-windows-msvc, strip = true suppresses PDB generation while delivering ~zero size benefit.

rustc's MSVC linker maps Strip::None to /DEBUG and both Strip::Debuginfo and Strip::Symbols to /DEBUG:NONE. So release builds currently emit aws_workload_credentials_provider.pdb; after this change they will not. MSVC keeps debug info in the PDB rather than in the EXE, so the EXE barely shrinks — the measurement in the PR description is explicitly x86_64-unknown-linux-gnu.

Failure scenario: staging.yml:56-58 builds the x86_64-pc-windows-msvc leg and staging.yml:119 uploads only aws-workload-credentials-provider.exe. A Windows Error Reporting minidump from the provider running as a service is unsymbolizable, and no PDB exists to match it — all of the debuggability cost, none of the size win. README.md:163 also documents cargo xwin build --release --target x86_64-pc-windows-msvc as a supported user path, which is now equally affected.

Note the Windows load-test checkbox in the PR description is still unchecked.

[secrets-manager-ai-code-review]

The bench profile inherits release, so this silently invalidates the repo's only automated performance guard.

Cargo: "The bench profile inherits the settings from the release profile." .github/workflows/benchmarks.yml:34 runs cargo bench --bench benchmark, so CacheHit/CacheEviction will now be built with fat LTO + codegen-units = 1.

Failure scenario: benchmarks.yml:50 reads the baseline from ./cache/benchmark-data.json, restored via restore-keys: benchmark-${{ runner.os }} (:40-42), and :54-55 set alert-threshold: '300%' with fail-on-alert: true. The first run after merge compares LTO-built numbers against a pre-LTO baseline. Because the threshold only fires on >300% regressions, a large apparent speedup is accepted silently and becomes the new baseline — so the profile change is never actually validated by the guard that exists, and the one-time discontinuity is baked in.

Worth invalidating the benchmark cache as part of this change so the discontinuity is explicit rather than absorbed.

[secrets-manager-ai-code-review]

Fat LTO can optimize the benchmark bodies away, because neither iter closure produces a value for criterion to black_box.

aws_secretsmanager_caching/benches/benchmark.rs:55-60:

b.to_async(&rt).iter(async || {
    cache.get_secret_value("secretid", None, None, false).await.unwrap();
});

The trailing ; makes the closure return (), so criterion's internal black_box on the return value has nothing to protect. Same shape at :100-105.

Failure scenario: today get_secret_value lives in a separate crate, so the optimizer cannot see through the call and the work survives. With lto = true the bench binary and aws_secretsmanager_caching are optimized as one unit — get_secret_value becomes inlinable and its result is provably unused, so the cache lookup can be dead-code-eliminated. CacheHit then measures an empty loop, reports a huge "improvement", and (per the fail-on-alert comment above) that becomes the permanent baseline — the perf guard is blinded rather than tripped.

Returning the value from the closure so criterion can black_box it fixes this and is worth doing before enabling LTO:

b.to_async(&rt).iter(async || {
    cache.get_secret_value("secretid", None, None, false).await.unwrap()
});

[secrets-manager-ai-code-review]

Wrong altitude: this overrides the shared release profile for everyone, when only the release-artifact build needs it.

The only places that actually build release are staging.yml:91 / :98 and Dockerfile:11. Everything else in CI is dev/test profile (rust.yml:65,67,69, integration-tests.yml build + test, test-local.sh:52,62, test-local.ps1:36,45, .vscode/launch.json), so those are unaffected — but every contributor following README.md:105 / :129 / :142 / :163 / :314 (cargo build --release) now pays a full fat-LTO, single-codegen-unit link over a 449-package dependency graph, as does anyone running cargo test --release or cargo bench.

A dedicated distribution profile confines the cost to the job that needs it and leaves cargo build --release fast and debuggable for everyone else:

Suggested change

	[profile.release]
	[profile.dist]
	inherits = "release"
	codegen-units = 1
	lto = true
	strip = "debuginfo"

…then build the artifact with --profile dist in staging.yml:91 / :98 and Dockerfile:11 (note the output path becomes target/<target>/dist/, so staging.yml:104, :119 and :137 need updating too).

[secrets-manager-ai-code-review]

Fat LTO over a 449-package graph is a real OOM risk on the release-build runners, and none of them have a cargo cache to fall back on.

Cargo.lock contains 449 packages (whole AWS SDK + tokio + rustls + aws-lc-sys). lto = true is fat LTO — the entire graph is merged into a single LLVM module at link time, and codegen-units = 1 then codegens it serially. Peak rustc/linker RSS for a graph this size commonly lands in the multi-GB range; GitHub-hosted ubuntu-latest and ubuntu-24.04-arm have 16 GB and 4 vCPU.

This lands hardest on the two cacheless cold builds:

• staging.yml:75-92 runs a fresh docker run of the AL2 image per job, installing the toolchain from scratch with an empty target/ — a full cold release build, now with fat LTO, on all three matrix legs (:50-58).
• docker.yml:34-40 uses docker/build-push-action with no cache-from/cache-to, so Dockerfile:11 (cargo build --release in rust:alpine) is a cold build on both linux/amd64 and linux/arm64 on every PR.

Failure scenario: LTO exhausts runner memory and the build is OOM-killed, or (more likely) it just gets much slower on every PR and every staging run with no cache to absorb it. lto = "thin" gets most of the optimization benefit at a fraction of the peak memory and link time, and is worth measuring against fat before committing — especially since the aarch64 leg is still unverified per the PR checklist.

[secrets-manager-ai-code-review]

codegen-units = 1 is unmeasured by the PR's own account and its cost lands on cacheless builds.

The description says it "significantly increases compilation time, is supposed to help with optimization" — i.e. the benefit is assumed, not measured. Meanwhile the headline 29MB→14MB result is attributable to strip, and lto = true already performs whole-program optimization, so codegen-units = 1 mostly just removes intra-crate codegen parallelism on top of that.

Concrete cost: it serializes codegen on 4-vCPU runners for the two cold, cacheless release builds (staging.yml:75-92, three matrix legs; docker.yml:34-40 → Dockerfile:11, two arch legs) plus cargo bench in benchmarks.yml:34. Since none of these have a cargo cache, that cost is paid in full on every PR and every staging run.

Suggest dropping it unless a measurement shows it earns its keep on top of LTO, or keeping it only in a dedicated dist profile as suggested on line 6.

[secrets-manager-ai-code-review]

The release-artifact build compiles the whole workspace, including integration-tests — and this profile makes that waste much more expensive.

staging.yml:91 (cargo build --release --target "$TARGET") and Dockerfile:11 (cargo build --release) build every workspace member, because the root manifest sets no default-members. integration-tests has a src/lib.rs, so it is a default member and gets built in release, dragging in reqwest 0.13, aws-sdk-sts, derive_builder, toml and tempfile (integration-tests/Cargo.toml:8-20) — none of which the shipped binary needs.

That was merely wasteful before; with codegen-units = 1 every one of those crates now codegens serially, and with lto = true they all emit bitcode. Multiply by three matrix legs in staging.yml:50-58 and two in docker.yml:16-22, on cacheless cold builds.

Cheap fix — scope the build to the shipped crate, e.g. cargo build --release --package aws_workload_credentials_provider --target "$TARGET" in staging.yml:91/:98 and Dockerfile:11, or add default-members = ["aws_workload_credentials_provider"] alongside members in this manifest.

[secrets-manager-ai-code-review]

No automated guard exists for the two targets the PR checklist admits are unverified — so the verification is hand-checked once and never again.

The description leaves stable-aarch64-unknown-linux-gnu and stable-x86_64-pc-windows-msvc load tests unchecked. There is nothing in CI that would catch a size or performance regression on either: benchmarks.yml:19 is runs-on: ubuntu-latest and benches only aws_secretsmanager_caching on x86_64 Linux, and no workflow sets timeout-minutes, so even a pathological build-time blowup surfaces only as slow jobs.

This is the shallow-vs-deep tension: a manually-ticked checkbox validates the profile once, for one target, at one commit. staging.yml:100-113 already runs a per-target post-build verification step (Verify glibc requirement) across all three legs — that is the natural place to hang a durable guard, e.g. a recorded binary-size budget per target, so future profile or dependency changes can't silently regress aarch64 or Windows.

[secrets-manager-ai-code-review]

The documented fips build is a release-mode path that CI never exercises, so this profile change ships untested against the AWS-LC FIPS module.

README.md:892 documents fips as a supported feature, built via cargo build --release --features fips, and README.md:83 documents a macOS workaround for it — so users do build it. The chain is aws_workload_credentials_provider/Cargo.toml:13 → aws_certificatemanager_provider/Cargo.toml:10 (aws-lc-rs/fips) → aws-lc-fips-sys (Cargo.lock:186).

The gap: fips is only ever compiled in CI via --all-features on dev-profile commands (rust.yml:67, :69, :79); rust.yml:65 builds without features and the release builds in staging.yml:91/:98 don't pass --features fips. So no job in this repo ever builds release + fips, and now none will build it with fat LTO either.

Failure scenario: the AWS-LC FIPS module performs a runtime power-on integrity self-check over its own text/rodata range. Fat LTO plus symbol stripping is precisely the class of link-time transformation that this check is sensitive to, and a failure surfaces only at process start on a customer's FIPS host — a build/runtime combination nothing here would catch. Worth a one-off cargo build --release --features fips before merging, and ideally a CI leg for it.

[secrets-manager-ai-code-review]

Adjacent risk this change surfaces: the glibc guard fails open if the symbol scan comes up empty.

staging.yml:101-113:

required=$(readelf --dyn-syms --wide "$BIN" | grep -oE 'GLIBC_[0-9]+\.[0-9]+' | sed 's/GLIBC_//' | sort -uV)
highest=$(echo "$required" | tail -n1)
if [[ "$(printf '%s\n%s\n' "$highest" "$AL2_GLIBC_VERSION" | sort -V | tail -n1)" != "$AL2_GLIBC_VERSION" ]]; then

If required is empty, highest is empty, printf '%s\n%s\n' "" "2.26" | sort -V | tail -n1 yields 2.26, the comparison equals $AL2_GLIBC_VERSION, and the guard reports success. The step is a plain run: block without pipefail, so a grep that matches nothing doesn't fail the pipeline either.

strip = true does not actually break this today — .dynsym/.gnu.version_r are SHF_ALLOC and survive --strip-all because the loader needs them. But this diff is exactly the class of change (altering which symbols exist in the binary) that would silently neuter the AL2 compatibility guard, and the guard would report green rather than erroring. Worth hardening while you're touching the build:

if [[ -z "$required" ]]; then
  echo "::error::found no GLIBC version references in $BIN — cannot verify glibc requirement"
  exit 1
fi