Skip to content

Trial PGO where it can be judged, not adopted on faith (#241) - #257

Open
kurok wants to merge 1 commit into
masterfrom
feat/241-pgo-ab
Open

kurok wants to merge 1 commit into
masterfrom
feat/241-pgo-ab

Conversation

@kurok

@kurok kurok commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Part of #241 — the workflow and the decision rule. The dispatches it asks for cannot run until this merges; see What is not here.

Branched from master (eea4874).

Why a trial rather than an adoption

PGO is easy to adopt on faith: the compiler gets a real profile, and the numbers usually move the right way. This crate is a bad place for faith.

Code placement alone moves its benchmarks up to 7.5% on these runners — measured in #240, not guessed — and PGO's mechanism is rearranging code. So a 4% "win" here is indistinguishable from a lucky layout draw unless it is measured against that floor.

What it does

Three wheels from one source and one toolchain — plain, -Cprofile-generate, -Cprofile-use — each with its own CARGO_TARGET_DIR, then plain against PGO interleaved on a single runner.

Training runs pytest tests --ignore=tests/benchmark (so malformed inputs and every mode are represented) and then pytest tests/benchmark --benchmark-disable, so the hot path is weighted the way production weights it rather than the way the tests do.

Two guards, because the failure mode here is a reassuring zero:

  • If training wrote no .profraw, the "optimised" build is silently identical to plain and the run reports 0% for the wrong reason. The job fails instead.
  • If the two extensions hash the same, the profile was not applied. Same.

llvm-profdata comes from rustc --print sysroot of the pinned toolchain, not the runner's PATH — a profraw written by one LLVM version is not readable by another's profdata, and the system one is not what rustc used.

The comparison runs in both orientations. ab_median.py flags a significant difference in whichever direction it is asked about, so a PGO win only announces itself when the question is put the other way round. That step is continue-on-error: true and its name says a red result is the adopt signal.

The decision rule is the point

In the workflow header and in CONTRIBUTING.md. Adopt only if the win clears the tolerance, exceeds the layout spread layout-ab.yml reports for the same revision, and reproduces on a run with a different Measured on CPU line.

Anything short of that is recorded as measured-and-not-adopted — a result, not a failure, and one that stops the next person re-running this.

CONTRIBUTING.md also states the question adoption would open and this trial does not answer: the PR gate builds without PGO, so its verdicts would stop describing the artefact users install. That parity problem belongs in the adoption issue.

And the trap I would have hit trying this locally: .cargo/config.toml sets rustflags for the apple targets, and a RUSTFLAGS environment variable replaces it rather than appending — so exporting RUSTFLAGS=-Cprofile-generate=... drops -C link-arg=-undefined dynamic_lookup and the link fails in a way that looks nothing like the cause.

What is not here

The dispatches. workflow_dispatch only works for workflows present on the default branch, so pgo-ab.yml cannot be run until this merges — the same constraint #249 hit. The acceptance criteria ask for two runs with different CPU lines, pasted here. I cannot produce those yet, and I would rather say so than imply the workflow has been exercised. It has never run. Its YAML parses and its structure mirrors toolchain-ab.yml and layout-ab.yml, both of which work, but that is not the same thing.

The outcome paragraph. Conditional on those numbers, so CONTRIBUTING.md states the rule and not a verdict.

Why this and not #239

#239 (lazy modes borrowing the pinned payload) is the more important remaining issue and I did not take it. It rewrites the retention model across the core and the binding — a Retained enum with offset arithmetic, refcounted buffer sharing between Python objects, and a documented behaviour change where the payload becomes pinned. That is not work to rush onto the end of a long session, on a file that #255 relocates wholesale. It wants its own pass with #255 landed first.

Checks run locally

pytest tests --ignore=tests/benchmark (852 passed, 3 skipped), ruff check ., and the workflow YAML parses with the documented inputs. git diff origin/master -- Cargo.toml src/ vendor/ .github/workflows/test.yml .github/workflows/publish.yml is empty.

Profile-guided optimisation is easy to adopt on faith: the compiler gets a
real profile, and the numbers usually move the right way. This crate is a bad
place for faith. Code placement alone moves its benchmarks up to 7.5% on the
runners -- measured, in #240 -- and PGO's mechanism *is* rearranging code. So
a 4% "win" here is indistinguishable from a lucky layout draw unless it is
measured against that floor.

pgo-ab.yml builds three wheels from one source and one toolchain -- plain,
instrumented, and optimised with the profile the instrumented build produced
-- and measures plain against PGO interleaved on one runner. Training runs the
whole suite, so malformed inputs and every mode are represented, then one pass
of the benchmark bodies so the hot path is weighted the way production weights
it rather than the way the tests do.

Two guards, because the failure mode here is a reassuring zero:

  If training wrote no .profraw, the optimised build is silently identical to
  plain and the run reports 0% for the wrong reason. The job fails instead.

  If the two extensions hash the same, the profile was not applied. Same.

llvm-profdata comes from `rustc --print sysroot` of the pinned toolchain, not
the runner's PATH: a profraw written by one LLVM version is not readable by
another's profdata, and the system one is not what rustc used.

The comparison runs in both orientations. ab_median.py flags a significant
difference in whichever direction it is asked about, so a PGO *win* only
announces itself when the question is put the other way round -- that step is
continue-on-error and its name says a red result is the adopt signal.

The decision rule is in the header and in CONTRIBUTING: adopt only if the win
clears the tolerance, exceeds the layout spread for the same revision, AND
reproduces on a second CPU. Anything less is recorded as measured-and-not-
adopted, which is a result rather than a failure -- it stops the next person
re-running this. CONTRIBUTING also states the question adoption would open and
this trial does not answer: the PR gate builds without PGO, so its verdicts
would stop describing the artefact users install.

Dispatch-only. Cargo.toml, publish.yml, test.yml, src/ and vendor/ are
untouched and the shipped wheels are unchanged.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>
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.

1 participant