ci: Use CodSpeed-hosted runners for perf CI - #4001
larseggert wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4001 +/- ##
==========================================
+ Coverage 96.84% 96.85% +0.01%
==========================================
Files 118 118
Lines 39989 39989
Branches 39989 39989
==========================================
+ Hits 38728 38732 +4
+ Misses 1237 1233 -4
Partials 24 24
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Merging this PR will regress 11 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | simulated/pacing-true/same-seed |
2.1 MB | 2.3 MB | -10.41% |
| ❌ | Memory | simulated/pacing-true/varying-seeds |
2.1 MB | 2.3 MB | -10.41% |
| ❌ | Simulation | coalesce_acked_from_zero 1000 ranges |
599.4 µs | 655.3 µs | -8.53% |
| ❌ | Simulation | coalesce_acked_from_zero 10 ranges |
9.6 µs | 10.3 µs | -7% |
| ❌ | Simulation | inbound_frame in-order |
1.2 ms | 1.3 ms | -6.18% |
| ❌ | Simulation | inbound_frame 5%-dup |
1.2 ms | 1.3 ms | -6.16% |
| ❌ | Simulation | coalesce_acked_from_zero 3 ranges |
3.5 µs | 3.7 µs | -6% |
| ❌ | Simulation | mark_sent retransmit |
33.1 µs | 34.9 µs | -5.19% |
| ❌ | Simulation | coalesce_acked_from_zero 1 ranges |
2.7 µs | 2.8 µs | -4.05% |
| ❌ | Simulation | write_frames 5-fair-streams all-active |
23.7 µs | 24.5 µs | -3.34% |
| ❌ | Simulation | inbound_frame 2%-loss |
2.3 ms | 2.4 ms | -3.17% |
| ⚡ | Simulation | simulated/pacing-true/varying-seeds |
116.1 ms | 74.8 ms | +55.25% |
| ⚡ | Simulation | simulated/pacing-false/varying-seeds |
115.1 ms | 74.4 ms | +54.7% |
| ⚡ | Simulation | simulated/pacing-false/same-seed |
116.5 ms | 75.5 ms | +54.31% |
| ⚡ | Simulation | simulated/pacing-true/same-seed |
115.8 ms | 75.1 ms | +54.21% |
| ⚡ | Memory | simulated/pacing-false/same-seed |
2.1 MB | 1.4 MB | +43.51% |
| ⚡ | Memory | simulated/pacing-false/varying-seeds |
2.1 MB | 1.4 MB | +43.51% |
| 🆕 | WallTime | walltime/1-streams/each-4194304-bytes |
N/A | 10.7 ms | N/A |
| 🆕 | WallTime | walltime/10-streams/each-1048576-bytes |
N/A | 28.6 ms | N/A |
| 🆕 | WallTime | 1-conn/1-100mb-req (aka. Upload)/mtu-1500 |
N/A | 48.7 ms | N/A |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing codspeed-macro-runners (51ac172) with main (fb2eb16)
Footnotes
-
7 benchmarks were run, but are now archived. If they were deleted in another branch, consider rebasing to remove them from the report. Instead if they were added back, click here to restore them. ↩
|
The new CodSpeed runners aren't faster and don't have tighter variances than our own. |
|
But there are more of them than our three. Let's give this another go. |
4f0cec6 to
6368d59
Compare
larseggert
changed the title
77b9f58 to
035b765
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
035b765 to
f050c3f
Compare
aa64d8c to
56699b3
Compare
bc62963 to
f12d70d
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Broad performance-methodology changes and unresolved CodSpeed setup inconsistencies require human validation.
Pull request overview
Migrates performance CI from self-hosted runners to CodSpeed-hosted hardware.
Changes:
- Reworks benchmark workflows, matrices, artifact handling, and profiling.
- Adds CodSpeed performance-comparison infrastructure and CPU pinning.
- Removes obsolete self-hosted runner provisioning.
File summaries
| File | Description |
|---|---|
| test/ansible/shrink.yml | Removes runner cleanup playbook. |
| test/ansible/README.md | Removes self-hosted runner documentation. |
| test/ansible/inventory.ini | Removes retired runner inventory. |
| test/ansible/install.yml | Removes runner provisioning playbook. |
| test/ansible/etc/sudoers.d.bench | Removes benchmark sudo rules. |
| test/ansible/etc/perfconfig | Removes runner perf configuration. |
| test/ansible/etc/journald.conf.d/99-neqo.conf | Removes runner journal limits. |
| test/ansible/etc/apt.conf.d/99-disable-periodic | Removes runner apt configuration. |
| test/ansible/bin/unprep.sh | Removes CPU restoration script. |
| test/ansible/bin/prep.sh | Removes CPU preparation script. |
| test-fixture/src/sim/connection.rs | Disables interface MTU discovery in simulations. |
| test-fixture/Cargo.toml | Reduces benchmark logging and NSS features. |
| neqo-udp/Cargo.toml | Reduces benchmark logging. |
| neqo-transport/src/lib.rs | Lowers crypto failure log level. |
| neqo-transport/Cargo.toml | Adjusts benchmark features and logging. |
| neqo-transport/benches/min_bandwidth.rs | Prints benchmark result despite log filtering. |
| neqo-qpack/Cargo.toml | Reduces benchmark logging. |
| neqo-http3/src/connection.rs | Lowers active-stream closure log level. |
| neqo-http3/Cargo.toml | Reduces benchmark logging. |
| neqo-common/Cargo.toml | Reduces benchmark logging. |
| neqo-bin/Cargo.toml | Reduces benchmark logging. |
| AGENTS.md | Documents the revised performance workflows. |
| .github/workflows/perfcompare.yml | Moves comparisons to CodSpeed runners. |
| .github/workflows/codspeed.yml | Uses CodSpeed macro runners and isolated Rust homes. |
| .github/workflows/codspeed-perfcompare.yml | Adds CodSpeed transfer benchmarks. |
| .github/workflows/bench.yml | Parallelizes benchmarks and aggregates results. |
| .github/scripts/perfcompare.py | Replaces cpusets with CPU affinity. |
| .github/scripts/format-bench-results.py | Removes obsolete cset filtering. |
| .github/dependabot.yml | Annotates the disabled update configuration. |
| .github/actions/process-perf/action.yml | Generalizes profile and report processing. |
| .github/actions/cpu-tuning/action.yml | Removes obsolete machine tuning action. |
| .github/actions/bench-matrix/action.yml | Includes all benchmark modes. |
| .github/actions/bench-machine/action.yml | Adds CodSpeed runner preparation. |
| .github/actionlint.yml | Registers the new runner label. |
Review details
- Files reviewed: 32/34 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
61140de to
3c2f01b
Compare
Pin benchmarks with taskset inside the CPU set the runner reserves, and set only the loopback MTU; the runners apply the rest of the tuning themselves and refuse elevated niceness. perf keeps sudo, which its tracepoints require, so only the profiling passes are privileged. Cap logging in the bench feature at warn, skip the interface MTU lookup for the simulator's unroutable peer address, and demote two per-packet warnings, to keep benchmark logs small. Share the perf post-processing between both workflows, build the peer implementations from one matrix job, and fetch their artifacts in one step. Drop test/ansible along with the on-prem runner it provisioned, and install SciPy for perfcompare.py in the workflow instead.
3c2f01b to
60b8b7b
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nice cleanup — trading ~280 lines of cpu-tuning plus the whole test/ansible/ tree for a runner image that already does the tuning is a big maintenance win, and the ephemeral-runner-per-job model removes a whole class of "stale state from the previous run" workarounds (clean: false, rm -rf target/criterion, fuser -k 4433, the restore step). The perfcompare.py refactor into Unit/run_unit/merge is clean, and deferring the SciPy import so --list-units/--merge work without it is a nice touch.
A few PR-wide observations, none of them blocking:
Fan-out cost. perfcompare.yml now runs 13 units and bench.yml one job per criterion variant, each paying a checkout, a toolchain install, install-samply, inferno, and an artifact download on a paid macro runner before doing any measuring. That per-job overhead is likely comparable to the measurement itself for the cheaper benches. Coarser units — one job per bench binary, or per client/server pair with the cc/pacing sweep inside — would keep most of the wall-clock win at a fraction of the billed minutes. Worth a look at the first full run's timings before settling.
Result-comparability break. MTU 1504 → 1500, new hardware, pmtud_iface_mtu(false) in the sim, and (see inline) renamed neqo-bin benchmarks all land at once, so no number in this PR is comparable to anything recorded before it, and the CodSpeed baselines for the affected benches reset. That's fine and probably unavoidable, but please call it out in the commit message so a future bisect doesn't chase the step change.
Duplication between the two results jobs. bench.yml and perfcompare.yml now end with near-identical jobs: checkout + git fetch of BASE_REF, download-artifact by pattern, format markdown, baseline-sha.txt, event.json, two upload-artifact exports, pr-comment-data-export, and "Fail on regression" grepping results.txt. That's ~60 duplicated lines that will drift; a reusable workflow taking the artifact pattern and the formatting command would collapse it.
Responses to existing review comments.
- Agreeing with Copilot's two comments about the missing job-local
RUSTUP_HOME/CARGO_HOME(bench.yml, perfcompare.yml): both jobs runinstall-build-depson the same runner image whose preinstalled toolchain motivated the workaround incodspeed.yml:54-61, so the same three lines belong there — or, better, move them intoinstall-build-depsitself so no future macro-runner job has to remember. - Copilot's codspeed-perfcompare MTU comment is resolved by the
MTU: 1500env plus themtu:input ontestdata-setup. - The zizmor
./neqo/...findings are resolved by the$/...conversion.
Minor: AGENTS.md gained the new perf workflows but not quic-build-cache.yml, and format-bench-results.py:236's "The runner keeps its workspace, so drop an earlier run's table" is now stale.
| mkdir -p profiles/neqo profiles/neqo-baseline stats/neqo stats/neqo-baseline | ||
| : > failed-benchmarks.txt | ||
|
|
||
| for benchmark in benchmarks/benchmark-*; do |
There was a problem hiding this comment.
Note
Glob expansion is lexicographic, so with ≥10 benchmarks the order is benchmark-0, benchmark-1, benchmark-10, benchmark-11, …, benchmark-2. format-bench-results.py emits all-bench-results.md in input order (process_input), so the "All results" list in the PR comment comes out shuffled.
| for benchmark in benchmarks/benchmark-*; do | |
| for benchmark in $(printf '%s\n' benchmarks/benchmark-* | sort -V); do |
|
|
||
| let achieved_bandwidth = TRANSFER_AMOUNT as f64 * 8.0 / simulated_time.as_secs_f64(); | ||
| qinfo!( | ||
| println!( |
There was a problem hiding this comment.
Warning
qinfo! went to stderr, but bench_exec … | tee -a ../results.txt captures stdout, so this line now lands in results.txt. format-bench-results.py treats every column-0 line as a new benchmark name, so each of the two invocations adds an empty <details> entry to the PR comment. eprintln! keeps it where qinfo! used to put it (the job log) without the survivorship in results.txt.
| println!( | |
| eprintln!( |
|
|
||
| [features] | ||
| bench = ["neqo-common/bench", "nss/bench", "blapi", "test-fixture/bench", "log/release_max_level_info"] | ||
| bench = ["neqo-common/bench", "blapi", "test-fixture/bench", "log/release_max_level_warn"] |
There was a problem hiding this comment.
Note
Capping at warn is what forces the two qwarn! → qdebug! demotions in library code (neqo-transport/src/lib.rs:243, neqo-http3/src/connection.rs:781). Those are real diagnostics — a failed crypto operation and close(0) with live streams — and downgrading them for benchmark noise costs signal in every consumer, Gecko included. release_max_level_error compiles the qwarn!s out of bench builds entirely and lets both call sites stay at qwarn!:
| bench = ["neqo-common/bench", "blapi", "test-fixture/bench", "log/release_max_level_warn"] | |
| bench = ["neqo-common/bench", "blapi", "test-fixture/bench", "log/release_max_level_error"] |
log's STATIC_MAX_LEVEL picks the first matching feature in off → error → warn → … order, so this only behaves predictably if every bench feature list in the workspace uses the same level.
Note
Why is nss/bench dropped here and in test-fixture/Cargo.toml? If it only set nss's own release_max_level_*, it was already a no-op (neqo's stricter cap wins), but that's worth a word in the commit message since nothing else documents it.
| run: cargo codspeed build --package "$CRATE" --locked --features bench --bench "$BENCH" --measurement-mode "$MODE" | ||
|
|
||
| - name: Set the loopback MTU # Same packetization as the other benchmark workflows. | ||
| run: sudo ip link set dev lo mtu "$MTU" |
There was a problem hiding this comment.
Warning
MTU is declared at workflow level, so it is also exported to the benchmark process, and neqo-bin/benches/main.rs:36 appends /mtu-$MTU to the bench name only when it is set. Every neqo-bin benchmark therefore gets renamed in CodSpeed and starts from an empty history. If only the interface needs the MTU, scope the variable to this step so the names are unchanged:
| run: sudo ip link set dev lo mtu "$MTU" | |
| - name: Set the loopback MTU # Same packetization as the other benchmark workflows. | |
| env: | |
| MTU: 1500 | |
| run: sudo ip link set dev lo mtu "$MTU" |
(and drop the workflow-level MTU). If the rename is intended, it'd be good to say so in the commit message — the CodSpeed baselines for those benches are lost either way.
| config: ${{ github.workspace }}/codspeed.yml | ||
| token: ${{ secrets.CODSPEED_TOKEN }} # zizmor: ignore[secrets-outside-env] | ||
|
|
||
| - name: Stop server |
There was a problem hiding this comment.
Warning
Nothing checks that the server outlived the 500 rounds. The non-neqo client commands from perfcompare-commands are bash -c "… >/dev/null 2>&1", so if the server dies mid-run every remaining round fails instantly and silently — walltime mode would record a large, meaningless improvement rather than an error. A one-liner before the teardown makes that loud:
| - name: Stop server | |
| - name: Verify the server survived the benchmark | |
| run: kill -0 "$SERVER_PID" | |
| - name: Stop server |
| perf script -i "$f" | inferno-collapse-perf --event-filter cycles | inferno-flamegraph --colors rust > "${f%.perf}.svg" | ||
| # Per-symbol IPC: divide the `instructions` section by `cycles:u`, both self cost. | ||
| perf report -i "$f" --stdio --no-children -g none \ | ||
| -F overhead,period,symbol,dso > "${f%.perf}.report.txt" |
There was a problem hiding this comment.
Note
Unlike the samply import above, these two aren't folded into fail. The step runs under bash -e -o pipefail, so one bad recording aborts the loop: the remaining files are never converted and the [ -z "$fail" ] accounting never runs. Same treatment as the line above keeps it best-effort but still fails the step:
| perf script -i "$f" | inferno-collapse-perf --event-filter cycles | inferno-flamegraph --colors rust > "${f%.perf}.svg" | |
| # Per-symbol IPC: divide the `instructions` section by `cycles:u`, both self cost. | |
| perf report -i "$f" --stdio --no-children -g none \ | |
| -F overhead,period,symbol,dso > "${f%.perf}.report.txt" | |
| perf script -i "$f" | inferno-collapse-perf --event-filter cycles | inferno-flamegraph --colors rust > "${f%.perf}.svg" || | |
| { echo "::error::flamegraph failed for $f"; fail=1; } | |
| # Per-symbol IPC: divide the `instructions` section by `cycles:u`, both self cost. | |
| perf report -i "$f" --stdio --no-children -g none \ | |
| -F overhead,period,symbol,dso > "${f%.perf}.report.txt" || | |
| { echo "::error::perf report failed for $f"; fail=1; } |
| [ -z "$MTU" ] || sudo ip link set dev lo mtu "$MTU" | ||
|
|
||
| # `libc6-dbg` lets samply symbolicate libc frames. | ||
| [ "$APT_UPDATED" ] || sudo apt-get update && echo "APT_UPDATED=1" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
Note
A || B && C parses as (A || B) && C, so C also runs when A succeeds — harmless here (a duplicate GITHUB_ENV line), but it reads as if-then-else and isn't. An explicit form also makes the perfcompare.yml copy of this idiom easier to keep in sync:
| [ "$APT_UPDATED" ] || sudo apt-get update && echo "APT_UPDATED=1" >> "$GITHUB_ENV" | |
| if [ -z "$APT_UPDATED" ]; then | |
| sudo apt-get update | |
| echo "APT_UPDATED=1" >> "$GITHUB_ENV" | |
| fi |
| grep -qw "$(hostname)" /etc/hosts || | ||
| echo "127.0.0.1 $(hostname)" | sudo tee -a /etc/hosts > /dev/null | ||
|
|
||
| [ -z "$MTU" ] || sudo ip link set dev lo mtu "$MTU" |
There was a problem hiding this comment.
Tip
With cpu-tuning gone, every measurement now rests on undocumented properties of the runner image (the ones the description lists). If CodSpeed changes it, results degrade with no signal at all — and samply silently loses kernel symbols if kptr_restrict isn't 0. A few exact-match assertions are cheap insurance:
| [ -z "$MTU" ] || sudo ip link set dev lo mtu "$MTU" | |
| [ -z "$MTU" ] || sudo ip link set dev lo mtu "$MTU" | |
| # The runner image is expected to have relaxed these; warn if it stops doing so. | |
| for WANT in /proc/sys/kernel/perf_event_paranoid=-1 /proc/sys/kernel/kptr_restrict=0; do | |
| IS=$(cat "${WANT%=*}") | |
| [ "$IS" = "${WANT#*=}" ] || echo "::warning::${WANT%=*} is $IS, expected ${WANT#*=}" | |
| done |
(smt/control is worth reporting too, but its value varies — off/forceoff/notsupported — so it needs a pattern rather than equality.)
|
|
||
| - name: Merge unit results | ||
| run: | | ||
| python3 neqo/.github/scripts/perfcompare.py --merge units --size "$SIZE" --runs "$RUNS" |
There was a problem hiding this comment.
Note
merge() globs whatever steps.md files exist, so a unit that failed just drops its row: the table posted to the PR silently under-reports, with the only hint being the red matrix job. bench.yml handles this explicitly with the benchmark-status/failed sentinel and the "Fail on benchmark errors" step — worth mirroring here, or at least having merge() compare the gathered units against units() and emit a ::warning:: (or a :x: row) for the missing ones.
Performance profiles for profiler.firefox.comBenchmarks (14)
|
Client/server transfer resultsPerformance differences relative to fb2eb16. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Table above only shows statistically significant changes. See all results below. All resultsTransfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
Benchmark resultsNo significant performance differences relative to fb2eb16. All resultstransfer/1-conn/1-100mb-resp (aka. Download)/mtu-1500 time: [49.983 ms 50.029 ms 50.076 ms]
thrpt: [1.9502 GiB/s 1.9520 GiB/s 1.9538 GiB/s]
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/1-conn/10_000-parallel-1b-resp (aka. RPS)/mtu-1500 time: [67.187 ms 67.469 ms 67.757 ms]
thrpt: [147.59 Kelem/s 148.22 Kelem/s 148.84 Kelem/s]transfer/walltime/pacing-true/varying-seeds time: [2.0672 ms 2.0693 ms 2.0719 ms]
thrpt: [1.8853 GiB/s 1.8877 GiB/s 1.8897 GiB/s]transfer/walltime/pacing-false/same-seed time: [2.0563 ms 2.0574 ms 2.0586 ms]
thrpt: [1.8975 GiB/s 1.8987 GiB/s 1.8997 GiB/s]transfer/walltime/pacing-true/same-seed time: [2.0581 ms 2.0590 ms 2.0600 ms]
thrpt: [1.8962 GiB/s 1.8972 GiB/s 1.8979 GiB/s]
Found 7 outliers among 100 measurements (7.00%)
7 (7.00%) high mildtransfer/1-conn/1-1b-resp (aka. HPS)/mtu-1500 time: [2.9219 ms 2.9229 ms 2.9239 ms]
thrpt: [342.01 B/s 342.13 B/s 342.25 B/s]
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high mildtransfer/1-conn/1-100mb-req (aka. Upload)/mtu-1500 time: [49.979 ms 50.015 ms 50.051 ms]
thrpt: [1.9511 GiB/s 1.9525 GiB/s 1.9539 GiB/s]
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildstreams/walltime/1-streams/each-1000-bytes time: [133.71 µs 134.30 µs 135.04 µs]
thrpt: [7.0621 MiB/s 7.1012 MiB/s 7.1323 MiB/s]
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildstreams/walltime/1000-streams/each-1-bytes time: [3.9028 ms 3.9126 ms 3.9216 ms]
thrpt: [249.02 KiB/s 249.60 KiB/s 250.22 KiB/s]
Found 20 outliers among 100 measurements (20.00%)
15 (15.00%) low severe
5 (5.00%) low mildstreams/walltime/1000-streams/each-1000-bytes time: [11.254 ms 11.261 ms 11.268 ms]
thrpt: [84.636 MiB/s 84.691 MiB/s 84.744 MiB/s]
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildstreams-flow-controlled/walltime/1-streams/each-4194304-bytes time: [10.557 ms 10.562 ms 10.569 ms]
thrpt: [378.47 MiB/s 378.70 MiB/s 378.90 MiB/s]
Found 5 outliers among 100 measurements (5.00%)
1 (1.00%) low mild
1 (1.00%) high mild
3 (3.00%) high severestreams-flow-controlled/walltime/10-streams/each-1048576-bytes time: [27.512 ms 27.529 ms 27.548 ms]
thrpt: [363.00 MiB/s 363.26 MiB/s 363.47 MiB/s]
Found 5 outliers among 100 measurements (5.00%)
2 (2.00%) high mild
3 (3.00%) high severetransfer/walltime/pacing-false/varying-seeds time: [2.0631 ms 2.0641 ms 2.0652 ms]
thrpt: [1.8915 GiB/s 1.8925 GiB/s 1.8934 GiB/s]Download data for |
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
|
We now have access to their
amd-ryzen-9950x-x86_64models.