diff --git a/.github/workflows/pgo-ab.yml b/.github/workflows/pgo-ab.yml new file mode 100644 index 0000000..4aee6c9 --- /dev/null +++ b/.github/workflows/pgo-ab.yml @@ -0,0 +1,201 @@ +name: PGO A/B + +# Does profile-guided optimisation earn its place in the shipped wheels? +# +# PGO is the kind of change that is easy to adopt on faith: the compiler gets a +# profile, the profile is real, the numbers usually go the right way. This crate +# is a bad place for faith. Code placement alone moves its benchmarks up to 7.5% +# on the runners (#240, measured), and PGO's whole mechanism is *rearranging +# code* -- so a 4% "win" here is indistinguishable from a lucky layout draw +# unless it is measured against that floor and reproduced on a second CPU. +# +# So this 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 in a single job. Same discipline as +# toolchain-ab.yml and layout-ab.yml: one runner, alternating rounds, the +# pure-Python benchmarks along as a noise floor. +# +# gh workflow run pgo-ab.yml -f rounds=5 -f tolerance=5 +# +# **Decision rule.** Adopt-candidate only if the PGO side wins by more than +# `tolerance` on the decoding benchmarks AND the win reproduces on a run with a +# different `Measured on` CPU line AND it exceeds the layout spread that +# layout-ab.yml reports for the same revision. Anything else is recorded in +# CONTRIBUTING.md as measured-and-not-adopted, which is a result, not a failure: +# it stops the next person re-running this. +# +# Adopting would also raise a question this workflow does not answer: the PR gate +# builds without PGO, so a gate verdict would no longer describe the artefact +# users install. That parity problem belongs in the adoption issue. +# +# Local trials on macOS: `.cargo/config.toml` sets `rustflags` for the apple +# targets, and a `RUSTFLAGS` environment variable *replaces* rather than appends +# to it -- so exporting RUSTFLAGS for a profile-generate build silently drops +# `-C link-arg=-undefined dynamic_lookup` and the link fails confusingly. Add the +# link args back by hand, or trial it here where the target has no such entry. + +on: + workflow_dispatch: + inputs: + tolerance: + description: "Delta (percent) a PGO gain must exceed, and a PGO loss must not" + default: "5" + rounds: + description: "Interleaved measurement rounds per side" + default: "5" + +permissions: + contents: read + +jobs: + pgo: + name: PGO vs plain + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.12" + cache: pip + + - name: Toolchain + run: | + python -m pip install -q --upgrade pip + # `llvm-tools` for llvm-profdata of *this* toolchain: a profraw written + # by one LLVM version is not readable by another's profdata, and the + # system llvm-profdata on the runner is not the one rustc used. + rustup component add llvm-tools + profdata="$(rustc --print sysroot)/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-profdata" + if [ ! -x "$profdata" ]; then + echo "::error::llvm-profdata not found at $profdata" + exit 1 + fi + echo "profdata=$profdata" >> "$GITHUB_ENV" + { + echo "### Toolchain" + echo + echo "- rustc: \`$(rustc --version)\`" + echo "- llvm-profdata: \`$("$profdata" --version | tr '\n' ' ')\`" + } >> "$GITHUB_STEP_SUMMARY" + + - name: Build plain (as shipped) + run: python -m pip wheel . --no-deps -w "$RUNNER_TEMP/w-plain" + + - name: Build instrumented + env: + RUSTFLAGS: -Cprofile-generate=${{ runner.temp }}/pgo-data + # A separate target dir per build is load-bearing: without it cargo + # reuses artefacts and a later "different" build is the same binary, + # which reports a reassuring 0% and means nothing. + CARGO_TARGET_DIR: ${{ runner.temp }}/target-instr + run: python -m pip wheel . --no-deps -w "$RUNNER_TEMP/w-instr" + + - name: Train + run: | + set -euo pipefail + python -m venv "$RUNNER_TEMP/venv-instr" + "$RUNNER_TEMP/venv-instr/bin/pip" install -q --upgrade pip + whl=$(ls "$RUNNER_TEMP"/w-instr/*.whl) + "$RUNNER_TEMP/venv-instr/bin/pip" install -q --no-cache-dir "${whl}[test]" + + # 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. + "$RUNNER_TEMP/venv-instr/bin/pytest" -q tests --ignore=tests/benchmark + "$RUNNER_TEMP/venv-instr/bin/pytest" -q tests/benchmark --benchmark-disable + + # An empty profile silently produces a PGO build identical to plain, + # and a run that reports 0% for the wrong reason. + count=$(ls "$RUNNER_TEMP/pgo-data"/*.profraw 2>/dev/null | wc -l) + if [ "$count" -eq 0 ]; then + echo "::error::training wrote no .profraw -- the instrumented wheel was not exercised" + exit 1 + fi + echo "- profraw files: $count" >> "$GITHUB_STEP_SUMMARY" + "$profdata" merge -o "$RUNNER_TEMP/merged.profdata" "$RUNNER_TEMP/pgo-data" + + - name: Build PGO-optimised + env: + # `-pgo-warn-missing-function` surfaces functions the profile does not + # cover instead of silently leaving them cold. + RUSTFLAGS: >- + -Cprofile-use=${{ runner.temp }}/merged.profdata + -Cllvm-args=-pgo-warn-missing-function + CARGO_TARGET_DIR: ${{ runner.temp }}/target-pgo + run: python -m pip wheel . --no-deps -w "$RUNNER_TEMP/w-pgo" + + - name: Check the two builds actually differ + run: | + set -euo pipefail + plain_whl=$(ls "$RUNNER_TEMP"/w-plain/*.whl) + pgo_whl=$(ls "$RUNNER_TEMP"/w-pgo/*.whl) + # Hash the extension, not the wheel: a wheel is a zip and its metadata + # can differ between byte-identical builds, which would make this guard + # pass when it should fail. + plain_hash=$(unzip -p "$plain_whl" '*.so' | sha256sum | cut -d' ' -f1) + pgo_hash=$(unzip -p "$pgo_whl" '*.so' | sha256sum | cut -d' ' -f1) + { + echo "- plain extension: \`${plain_hash:0:16}\`" + echo "- pgo extension: \`${pgo_hash:0:16}\`" + } >> "$GITHUB_STEP_SUMMARY" + if [ "$plain_hash" = "$pgo_hash" ]; then + echo "::error::PGO produced an identical extension -- the profile was not applied" + exit 1 + fi + echo "plain_whl=$plain_whl" >> "$GITHUB_ENV" + echo "pgo_whl=$pgo_whl" >> "$GITHUB_ENV" + + - name: Install both builds + run: | + set -euo pipefail + for side in plain pgo; do + python -m venv "$RUNNER_TEMP/venv-$side" + "$RUNNER_TEMP/venv-$side/bin/pip" install -q --upgrade pip + done + "$RUNNER_TEMP/venv-plain/bin/pip" install -q --no-cache-dir "${plain_whl}[test]" + "$RUNNER_TEMP/venv-pgo/bin/pip" install -q --no-cache-dir "${pgo_whl}[test]" + + - name: Measure, interleaved + run: | + set -euo pipefail + for round in $(seq 1 "${{ inputs.rounds }}"); do + for side in plain pgo; do + echo "::group::round $round — $side" + "$RUNNER_TEMP/venv-$side/bin/pytest" -q tests/benchmark \ + --benchmark-min-rounds=25 \ + --benchmark-json="$side-$round.json" + echo "::endgroup::" + done + done + + - name: Is PGO slower than plain? + env: + AB_THRESHOLD_PCT: ${{ inputs.tolerance }} + run: | + python .github/scripts/ab_median.py \ + --a-label pgo --b-label plain \ + --informational test__threaded___ \ + --a pgo-*.json --b plain-*.json + + # Red here is the ADOPT signal, not a failure: `ab_median.py` flags a + # significant difference in whichever direction it is asked about, so + # running it the other way round is how a PGO *win* announces itself. + - name: Is PGO faster than plain? (a red result here is the adopt signal) + continue-on-error: true + env: + AB_THRESHOLD_PCT: ${{ inputs.tolerance }} + run: | + python .github/scripts/ab_median.py \ + --a-label plain --b-label pgo \ + --informational test__threaded___ \ + --a plain-*.json --b pgo-*.json + + - name: Upload the reports and the profile + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: pgo-ab-reports + path: | + *-[0-9].json + ${{ runner.temp }}/merged.profdata + if-no-files-found: warn diff --git a/CHANGELOG.md b/CHANGELOG.md index c3ae251..891ca3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **A dispatch-only PGO A/B** (#241). Profile-guided optimisation is easy to adopt on + faith -- the compiler gets a real profile and the numbers usually move the right way -- + and this crate is a bad place for faith: code placement alone moves its benchmarks up to + 7.5% on the runners (#240, measured), 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. `gh workflow run pgo-ab.yml` builds three wheels from one source and one + toolchain -- plain, instrumented, and optimised with the profile that instrumented build + produced from the whole test suite plus one pass of the benchmark bodies -- and measures + plain against PGO interleaved on a single runner, in both orientations so a win + announces itself as loudly as a loss. It fails the job if training wrote no `.profraw`, + or if PGO produced a byte-identical extension, since both would report a reassuring 0% + for the wrong reason. The decision rule is recorded in `CONTRIBUTING.md`: adopt only if + the win clears the tolerance, exceeds the layout spread for the same revision, and + reproduces on a second CPU. Nothing about the shipped wheels changes. + - **A layout A/B, so code placement can be measured instead of argued about** (#240). This crate has been bitten by placement four times: a rustc minor version moved the parse path 15-96% (#120), a package-version bump did the same for a byte-identical diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c81f39d..e8742f8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -422,6 +422,40 @@ A profiling build is a **different binary** from the release one -- line tables change section sizes and therefore layout, and this crate is measurably sensitive to layout. Use it to find out *where* the time goes, never how much. +### Is PGO worth it? + +Unanswered deliberately, and there is a workflow to answer it: + +```sh +gh workflow run pgo-ab.yml -f rounds=5 -f tolerance=5 +``` + +It builds three wheels from one source and one toolchain -- plain, instrumented, +and optimised with the profile that instrumented build produced by running the +whole test suite plus one pass of the benchmark bodies -- then measures plain +against PGO interleaved in a single job, in both orientations so a win announces +itself as loudly as a loss. + +**The decision rule matters more than the number.** PGO's mechanism is +rearranging code, and rearranging code is exactly what moves this crate's +benchmarks for no reason at all -- see the layout A/B below, which measured up to +7.5%. So a PGO win counts only if it clears `tolerance`, **exceeds the layout +spread for the same revision**, and reproduces on a run with a different +`Measured on` CPU line. Anything short of that is recorded here as +measured-and-not-adopted, which is a result: it stops the next person re-running +it. + +Adopting it would raise a question the workflow 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, not the trial. + +A trap for local trials on macOS: `.cargo/config.toml` sets `rustflags` for the +apple targets, and a `RUSTFLAGS` environment variable **replaces** it rather than +appending. Exporting `RUSTFLAGS=-Cprofile-generate=...` therefore drops +`-C link-arg=-undefined dynamic_lookup`, and the link fails in a way that looks +nothing like the cause. Add the link args back by hand, or trial it on the linux +runner, where the target has no such entry. + To measure it rather than argue about it, dispatch the layout A/B: ```sh