(07) bench - #1759
Draft
daniel-noland wants to merge 13 commits into
Draft
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
2 times, most recently
from
August 26, 2026 19:36
640edd0 to
47d1350
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
4 times, most recently
from
August 26, 2026 21:25
afe8933 to
ab17860
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
from
August 27, 2026 01:29
ab17860 to
b0a5e97
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
from
August 27, 2026 01:41
b0a5e97 to
c2c48ab
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
from
August 27, 2026 04:35
c2c48ab to
61fb4c7
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
from
August 27, 2026 05:12
61fb4c7 to
b18bba9
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
from
August 27, 2026 18:00
b18bba9 to
9a66fc6
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
2 times, most recently
from
August 28, 2026 02:13
4517822 to
bb9404d
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
from
August 28, 2026 02:19
bb9404d to
54309b8
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
from
August 28, 2026 03:05
54309b8 to
b721f06
Compare
daniel-noland
changed the title
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
from
August 28, 2026 17:16
2777622 to
3b54931
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
from
August 28, 2026 17:33
3b54931 to
71b529f
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
2 times, most recently
from
September 3, 2026 00:21
02abb05 to
620c914
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
from
September 3, 2026 05:53
620c914 to
61f68c1
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
2 times, most recently
from
September 3, 2026 06:22
abf1401 to
a61ef9b
Compare
`Fib::lpm_entry_prefix` walks a route's fib groups twice -- once to count entries, once to turn the hashed index back into a group and an offset -- which looks like a per-packet cost worth removing. It is not. Caching the count regresses the common case. One group's two walks touch the same cache lines, so the second is nearly free, while the extra field grows `FibRoute` from 24 to 32 bytes and costs more in the trie than it saves: 1g x1e goes from 17.1ns to 17.5ns. It only pays from about eight groups up, reaching -16% at 16g x1e and -22% at 16g x4e. Whether that trade is worth making is a question about how wide real ECMP gets, and the answer is not in the code. The proportions are the useful part: of 17.1ns, the trie lookup is 10.0 and pulling the destination out of the packet is 2.3, so entry selection is under a third of a lookup that is itself mostly trie. The assertion inside the loop is load-bearing rather than decorative. The test packet is addressed to 5.6.7.8, so a route installed on 10.0.0.0/8 sends every shape to the default route and returns the same number -- a bench that measures nothing and says so nowhere. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Puts callgrind next to the criterion bench it already has, so a benchmark can count work as well as time it, and calibrates the two against each other while there is a known answer to calibrate against. Taking the caching change from the previous commit and measuring it both ways: callgrind reports -4.3% instructions on a one-group route where the machine is 3.1% slower. It counts the instructions the change removes and cannot see that `FibRoute` grew from 24 to 32 bytes, or that the walk it removed was hitting L1 anyway. Its modelled cache does not catch it either -- a fixed generic L1 that this fixture fits in whichever layout it has. Where the change is algorithmic rather than structural the two agree on sign and land within a small factor. So it is worth gating on and not worth deciding with: instruction counts are bit-identical run to run, and blind to layout. Both halves are in development/code/benchmarking.md, along with the two ways these benchmarks were wrong first -- a fixture that missed the code under test, and a measured function that owned its fixture and so mostly timed the drop. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…they lie Three things worth knowing before somebody needs one of these in a hurry. The family is wired up and exercised, so reaching for DHAT or massif is a two-line edit. Only callgrind scopes to the benchmark function, though: cachegrind has no call graph and counts the whole process, reporting 453,358 instructions against callgrind's 349 for the same benchmark. Cachegrind can be handed the real cache geometry and now is, but it already reads CPUID and gets L1 right unaided; what it gets wrong is collapsing L2 and L3 into one "LL" and guessing 8 MB against this host's 32 MB. That changed nothing here, because the fixture never leaves L1 -- an argument for realistic fixture sizes rather than for tuning the model. DPDK is the one that matters. Valgrind does not fall over on rte_acl: the whole ACL suite runs, 39.5 billion instructions, no crash. It instead reports a CPU it can emulate, so DPDK's runtime dispatch chose scalar and AVX2 and executed no AVX-512 at all -- on a Zen 4 part that has it, and would use avx512x16/x32 in production. That is not a modelling error to correct for; it is a measurement of a different function. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each bench had its own copy of the fixture builder, the measured body and the list of route shapes. Two benchmarks that drift apart do not fail -- they answer different questions while still looking comparable, which turns the calibration between them into a lie that compiles. `benches/common` now holds all three. The shape list is a macro rather than a `const` because the callgrind harness needs one `#[bench::id(..)]` per shape and attributes cannot be looped over, so one definition expands into an array for criterion and into attributes for callgrind. Sharing the body also fixed an asymmetry that was already there: criterion hoisted the read guard out of its loop and callgrind did not, so the pair had been measuring slightly different regions all along. Both include it now, with `enter_only` to subtract. Callgrind reports "No change" on every counter across the refactor, which is the check that the shared body is the same code. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`just bench-compare` prints a headline, a table of every metric, and a bar chart when something moved past a threshold. The report needs no memory of previous runs, which is the part worth explaining. Storing yesterday's numbers would make the answer depend on which runner recorded them. Callgrind counts work rather than measuring time, so the comparison only requires both sides to run on the same machine -- and they can do so in the same job: base with `--save-baseline`, head with `--baseline`. Each record then carries both values and the delta already, and nothing outlives the run. The full report belongs in `$GITHUB_STEP_SUMMARY` with the headline as a sticky comment linking to it. A gist would need a personal access token with `gist` scope, since `GITHUB_TOKEN` cannot create one -- a credential to rotate in exchange for an artifact that is neither permalinked to the run nor able to render mermaid. The workflow itself is described rather than written: it cannot be exercised from here, and an untested workflow that comments on every pull request is worse than a documented one somebody runs once by hand. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the report against a real change -- the FibRoute caching experiment -- turned up two things a synthetic test could not. The headline was picking whichever number moved most, which meant `EstimatedCycles` beat `Ir` by four hundredths of a percent and put the least trustworthy metric at the top. It is a formula over the counters, not a time, and it moves alongside `Ir` without adding anything. More useful: DHAT sees what callgrind cannot. That change grows `FibRoute` from 24 bytes to 32, which is invisible to an instruction count and is the whole reason it loses on a real machine -- but allocation totals move, 7,226 bytes to 7,354. So the headline now names both, and "less work, more memory" is called out in the guide as the shape of change most likely to read as a win here and lose in production. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follows `ci` and `miri`: `just bench criterion`, `just bench callgrind`, `just bench compare`, `just bench baseline`. A module cannot share a name with a recipe, so the criterion one moved too and now invokes `just build benches` rather than depending on it. Which turned up a bug the flat layout had hidden. The nix `benches` build produces `fib_lookup_callgrind` alongside the criterion binaries, and the loop ran everything it found -- so `just bench` was respawning the callgrind target under valgrind and overwriting whatever baseline `compare` had stored. The loop now skips `*_callgrind`, which is what the naming convention was for. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Criterion's html report needs `html_reports` and a `plotters` backend, neither of which survived `default-features = false`. Both members wanted the same set, so it moves to the workspace. `rayon` stays off: it parallelises sampling, trading run-to-run stability for wall-clock we do not need back. Opening the result from `file://` does not work -- the pages fetch their siblings relatively and the origin refuses, so the index renders and everything under it is empty. `static-web-server` joins the dev shell, with a generic `just serve` behind `just bench serve` and `just serve-coverage`, since llvm-cov's report has the same problem. Verifying that turned up something worse than the missing feature. The nix `benches` build inherits the root justfile's `profile`, which is `debug`, so this recipe has always benchmarked unoptimised binaries: the same benchmark reads 20.9ns there against 2.35ns from a release build. Nine times slow, no inlining, and wrong in the way that still looks like a measurement. The module now asks for `release`. `cargo bench` was never affected -- its profile inherits from `release` -- so the numbers reported from it stand. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
They went to the nix build, where nobody wants them, leaving no way to scope a run. A full sweep is over half an hour -- nearly all of it the rte_acl benches walking fifteen rule counts -- so iterating on one benchmark meant waiting for all of them. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
static-web-server defaults to `::`, so `just serve-coverage` was offering the whole source tree to anything that could reach the port. Turned up while checking the html reports serve at all, which they now do end to end: index, per-benchmark pages, and plots. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`iai-callgrind` serializes its results with `bincode` 1.3.3, so adding the callgrind harness made `check-dependencies` fail on RUSTSEC-2025-0141. There is nothing to upgrade to: the advisory records a team that stopped deliberately and considers 1.3.3 complete. The entry states the invariant that makes it tolerable -- dev-dependency only, absent from every shipped artifact -- so a later reviewer can re-check that rather than re-derive it. Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`compare` tested `-d target/iai`, which `just bench callgrind` also creates, and `--baseline=name` exits 0 when `name` is absent rather than failing. Between them the save branch was unreachable once anything had written to `target/iai`: a `compare` against a name that was never recorded reported "recorded a new one" and recorded nothing, so the next run had nothing to compare against either. Signed-off-by: Daniel Noland <daniel@githedgehog.com>
A just module is a closed scope: it does not inherit the root justfile's variables and there is no command-line syntax that sets one. So moving these recipes into `bench.just` and replacing the `(build "benches")` dependency with a `just build benches` subprocess put every build argument out of reach -- `features`, `platform`, `libc`, `kernel`, `default_features`, `instrument`, `jobs`, `cores`. The `just features=dpdk bench criterion` the module's own documentation asked for reached nix as `--argstr features ""`, so the rte_acl benches it names could not be run at all. Back to flat recipes and a real dependency, which shares this invocation's variables. `just bench <sub>` becomes `just bench-<sub>`, as it was before, and the module's genuine fixes stay: skipping `*_callgrind` in the criterion loop, the callgrind target variables, the html report, and `bench-baseline`. The module also carried `profile := "release"`, which a flat recipe cannot have without diverging from the global knob every other recipe reads. `bench` refuses any other profile instead of quietly benchmarking a debug build. Signed-off-by: Daniel Noland <daniel@githedgehog.com>
daniel-noland
force-pushed
the
pr/daniel-noland/bench-harness
branch
from
September 3, 2026 07:02
a61ef9b to
f9e99f9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.