feat: Add some benches and integrate with CodSpeed#62
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62 +/- ##
=======================================
Coverage 97.03% 97.03%
=======================================
Files 2 2
Lines 776 776
Branches 776 776
=======================================
Hits 753 753
Misses 21 21
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
9b2b79b to
5176589
Compare
Congrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
There was a problem hiding this comment.
Pull request overview
Adds a benchmark suite for the happy-eyeballs state machine and wires it into CI via CodSpeed to track performance over time.
Changes:
- Added
divan-style benchmarks covering several Happy Eyeballs scenarios (IPv6-only, dual-stack racing, HTTPS/ECH, many endpoints). - Integrated CodSpeed-compatible Divan dependency and registered the
happy_eyeballsbench target. - Updated CI to run CodSpeed benchmarks and expanded coverage command to include all targets.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
benches/happy_eyeballs.rs |
New Divan/CodSpeed-compatible benchmark scenarios for the state machine. |
Cargo.toml |
Adds CodSpeed Divan compat dev-dependency and declares the bench target (no harness). |
Cargo.lock |
Locks new transitive dependencies introduced by CodSpeed/Divan compat. |
.github/workflows/rust.yml |
Runs cargo bench --bench happy_eyeballs via CodSpeed; adjusts coverage run to --all-targets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5176589 to
2d0d58c
Compare
b12ebab to
dd4addb
Compare
dd4addb to
113191e
Compare
|
@mxinden ready from my side. Can tweak the actual benches in follow-up PRs. |
There was a problem hiding this comment.
Good addition — the benchmarks are well-structured, cover the key state machine paths (single-family, dual-stack racing, HTTPS/ECH, many-endpoint exhaustion), and the CodSpeed integration is straightforward.
A few things to address:
-
Action version mismatch: The
benchmarksjob still pinsmozilla/actions/rustat v1.0.4 while the rest of the workflow was bumped to v1.0.11 via main. See inline comment. -
--all-targetsin coverage job: This change causes the benchmark binary to also execute during coverage runs (divan falls back to walltime mode outside CodSpeed). Worth considering whether that's intentional — see inline comment. -
PR title typo: "Cosdpeed" → "CodSpeed".
-
Re: Copilot's
None-in-loop finding (comment): This was already addressed — the current code correctly groupsNonewith thebreakarm (lines 253–256). -
Re: zizmor
permissionsfinding (comment): Already addressed —id-token: writehas the explanatory comment on line 50.
|
@mxinden I don't think this will cause vendoring issues for Gecko. |
|
@mxinden OK to merge? |
There was a problem hiding this comment.
Good set of benchmarks — they cover the key state machine paths (single-family, dual-stack racing, HTTPS/ECH, many-endpoint exhaustion) and the CodSpeed integration is clean.
Most findings from prior reviews have been addressed in the latest push: --all-targets removed from coverage (replaced with cargo check --benches), String::new() for failure messages, trailing newline in Cargo.toml, and None correctly breaks in the many_endpoints loop.
One remaining item: the mozilla/actions/rust pin in the benchmarks job is still behind — v1.0.11 vs v1.1.0 in the rest of the workflow (see inline comment).
PR title typo (noted by the prior review but still present): "Cosdpeed" → "CodSpeed".
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Well-designed benchmark suite covering the key state machine paths (single-family, dual-stack racing, HTTPS/ECH, many-endpoint exhaustion). The CodSpeed integration and CI wiring are clean.
Blocking: The latest commit ("Fix") breaks CI — cargo check --benches and cargo codspeed build both fail. See inline comment for the fix.
Previously flagged issues — now resolved:
mozilla/actions/rustpin: now v1.1.0, consistent with the rest of the workflow--all-targetsin coverage: replaced with a separatecargo check --benchesstepString::new()for failure messages (avoids allocation noise)- Trailing newline in
Cargo.toml Nonecorrectly breaks in themany_endpointsloop
922db37 to
4aa2e2a
Compare
There was a problem hiding this comment.
Well-structured benchmark suite covering the key state machine paths — good selection of scenarios (IPv6-only baseline, dual-stack racing, HTTPS/ECH with hints, many-endpoint exhaustion).
Status of prior review findings — all addressed in latest push:
assert_matchesimport (prior comment): Fixed — switched to stableassert!(matches!(…)), removing the brokenuse std::assert_matchesthat prevented compilation.mozilla/actions/rustpin: Now v1.1.0, consistent with the rest of the workflow.--all-targetsin coverage: Replaced with a dedicatedcargo check --benchesstep — cleaner separation of compile-check vs. coverage.String::new()for failure messages: Eliminates per-iteration allocation noise.- Trailing newline in
Cargo.toml: Present.
No new blocking issues found. A few observations:
- The
many_endpointsloop driver (lines 228–241) is well-designed: it correctly breaks on terminal outputs (Succeeded/Failed) andNone, and theVecDeque-based in-flight tracking naturally handles the interleavedAttemptConnection/Timercadence the state machine produces. - The code duplication between
benches/happy_eyeballs.rsandtests/common/mod.rs(constants, input constructors) was noted in a prior review — still present but non-blocking and the bench helpers are intentionally leaner (noenv_logger,drain_dns_queriesbaked into setup). - CI permissions are properly scoped (
id-token: writeonly on thebenchmarksjob, with an explanatory comment). Theworkflow_dispatchtrigger for CodSpeed backtesting is standard.
|
Sorry for the delay. The state machine is quite simple. The most expensive operations are a couple of string allocations. I'm not convinced these benchmarks provide much value at this point. If the result is X, what does X tell us relative to? We haven't identified any improvement opportunities to measure against. I'd suggest holding off on merging until we've identified something concrete to improve, e.g. if happy eyeballs shows up in a Firefox profile. Until then, I suggest we keep this as a pull request only. I should have suggested this earlier, sorry about that. |
|
ACK, I'll make it a draft. |
|
(Wondering if the memory benchmarks would be more useful...) |
No description provided.