Repository navigation
bench: criterion suite for the extension hot paths - #1474
duncanista wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 341f23b39d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fi | ||
| echo 'machine:' \$(uname -sm) \$(nproc) 'cpus visible,' \$(grep -m1 'model name' /proc/cpuinfo | cut -d: -f2- || true) | ||
| echo 'profile:' \${CARGO_PROFILE_BENCH_LTO:+bench FAST (no LTO, NOT production)}\${CARGO_PROFILE_BENCH_LTO:-bench = release (opt-level z, lto, codegen-units 1)} | ||
| cargo bench ${benches[*]-} -- --save-baseline '$name' ${extra[*]-} |
There was a problem hiding this comment.
Because the collected arguments are interpolated as unquoted text into bash -c, valid Criterion filters containing spaces or shell metacharacters are re-parsed by the container shell. For example, scripts/bench-docker-arm64.sh main logs -- 'foo|bar' turns the filter into a pipeline rather than passing foo|bar to Criterion, so the benchmark fails or runs the wrong command. Pass the arrays as positional parameters to the inner shell (or otherwise shell-quote each element) instead of embedding ${extra[*]} and ${benches[*]} in the command string.
Useful? React with 👍 / 👎.
|
|
||
| here="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" | ||
| crate="$here/bottlecap" | ||
| export CARGO_TARGET_DIR="${CARGO_TARGET_DIR:-$crate/target}" |
There was a problem hiding this comment.
When CARGO_TARGET_DIR is relative, as in the documented CARGO_TARGET_DIR=bottlecap/target-linux-arm64 scripts/bench-compare.sh compare ... command, it is exported unchanged and then Cargo or critcmp runs after cd "$crate". Cargo consequently resolves that example as bottlecap/bottlecap/target-linux-arm64, while the metadata checks and writes later in this script resolve it from the caller's directory as bottlecap/target-linux-arm64; saved results, notes, and comparisons therefore end up looking in different directories. Convert a supplied relative path to an absolute path before entering the crate.
Useful? React with 👍 / 👎.
| (cd "$crate" && cargo bench ${benches[@]+"${benches[@]}"} -- --save-baseline "$name" ${extra[@]+"${extra[@]}"}) | ||
| mkdir -p "$CARGO_TARGET_DIR/criterion" | ||
| printf '%s\nsaved: %s\ngit: %s\n' "$note" "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ | ||
| "$(git -C "$here" rev-parse --short HEAD 2>/dev/null || echo unknown)" \ |
There was a problem hiding this comment.
In the documented before/after workflow, the optimized code is commonly benchmarked immediately after it is edited but before it is committed. rev-parse HEAD then records the same commit for both baselines even though the second binary contains working-tree changes, making the saved metadata misattribute the result and preventing later reproduction. Include the dirty state (and ideally a diff or tree identifier) in the note rather than recording only HEAD.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
duncanista
changed the title
- bench-docker-arm64.sh: pass the cargo arguments to the container shell as positional parameters, so criterion filters are not re-parsed. - bench-compare.sh: make a relative CARGO_TARGET_DIR absolute before cd, so cargo, critcmp and the baseline notes use the same directory. - bench-compare.sh: record a dirty marker and a hash of the uncommitted diff in the baseline notes. - benches/README.md: drop the old branch name.
TL;DR: Criterion benchmarks for the hot paths: log processing, trace processing, invocation parsing, enhanced metrics, config load and init.
Why: The extension runs on about 0.07 vCPU at 128 MB, so per-line and per-span costs show up in the customer's billed duration. We need before and after numbers for each performance fix.
Overview
Jira: SVLS-9977
Stacked on #1473 (
jordan.gonzalez/fault-harness/feature). It needs the fixtures and the dependency removal; the harness PR is only stacking order.The extension runs at about 0.07 vCPU at 128 MB, so per-line, per-span and per-invocation
costs show up in billed duration. This PR adds 6 criterion benches (
bottlecap/benches/):logs,traces,invocation,enhanced_metrics,config,init. Benches namedalt_*measure an alternative from the review, as the target for a fix.
bottlecap/benches/README.md: what each bench measures, how to run it, noise control, thefirst baseline, and a table from each review item to its bench and number.
scripts/bench-compare.sh: save and compare criterion baselines with critcmp.scripts/bench-docker-arm64.sh: the suite in a 2-CPU Linux arm64 container.Cargo.toml:criterion0.5 as a dev-dependency (no plotters, no rayon), the 6[[bench]]targets, and
bench = falseon the lib and binaries socargo benchruns only criterion.#[doc(hidden)] pubso the benches can reach them(
is_oom_error, the/procreaders*_from_path). No behavior change..gitignore: criterion exports and the Linux arm64 target dir.Size: 270 core lines, 3032 lines in total.
Testing
cargo fmt --all -- --check: passcargo metadata --locked: passcargo clippy --workspace --all-targets --features default -- -D warnings: passcargo clippy --workspace --all-targets --features default,test-mode -- -D warnings: passcargo nextest run --workspace: passcargo bench --no-run(LTO off, 16 codegen units): passcargo deny --locked check: passcargo machete: passdd-rust-license-tool check: passcargo clippy --workspace --all-targets --no-default-features --features fips -- -D warnings: pass. It cannot run on macOS (libddwaf-sys load error).