Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions .github/workflows/perf-pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,14 @@ jobs:
- name: Measure this pull request
run: mise run perf:record

# Reported, never gated: the shadow is a fixture that grows as the derive learns to
# express more, so a gate on it fires for a bigger fixture rather than a slower
# parser. `|| true` because an informative table is not worth failing a run over.
# The script rather than the task: the release build is already done by the step
# above, and the task exists for running this by hand.
- name: Measure the shadow against clap
run: ./tasks/perf-shadow.sh /tmp/shadow-report.md || true
Comment thread
greptile-apps[bot] marked this conversation as resolved.

- name: Find the base commit
id: base
env:
Expand Down Expand Up @@ -123,6 +131,9 @@ jobs:
# a different failure from a regression and must not be reported as
# one. The gate below maps them to different messages.
cp /tmp/tak-gate-status /tmp/tak-out/status 2>/dev/null || echo 2 > /tmp/tak-out/status
# Absent when valgrind is missing or the measurement failed, which the reporting
# job treats as nothing to add rather than as an error.
cp /tmp/shadow-report.md /tmp/tak-out/shadow.md 2>/dev/null || true
printf '%s %s\n' "${HEAD_SHA:0:12}" "${BASE_SHA:0:12}" > /tmp/tak-out/shas

- uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7
Expand Down Expand Up @@ -168,6 +179,12 @@ jobs:
printf '%s\n' "$marker"
printf '### Instruction counts\n\n'
cat /tmp/tak-out/report.md
# The shadow comparison, when there is one: absent on a runner without
# valgrind, and nothing to apologise for when it is.
if [ -f /tmp/tak-out/shadow.md ]; then
printf '\n'
cat /tmp/tak-out/shadow.md
fi
printf '\n<sub>`%s` vs `%s` · measured on the runner, not pushed to the history.</sub>\n' \
"$head_sha" "$base_sha"
} > /tmp/tak-comment.md
Expand Down
6 changes: 6 additions & 0 deletions mise.toml
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,12 @@ run = [
"cargo run -q -p xtask -- gen-shadow benches/mise.usage.kdl benches/shadows/mise-clap clap",
]

# The shadow comparison: usage against clap, at mise's scale. Reported, never gated — see
# the note in tak.toml. Takes an optional path to write the markdown to.
[tasks."perf:shadow"]
dir = "{{config_root}}"
run = ["cargo build --release", "./tasks/perf-shadow.sh"]

# Instruction-counted benchmarks (see tak.toml). Needs valgrind for the counts;
# without it tak reports wall clock only and says so, so this still works on
# macOS where there is no usable cachegrind.
Expand Down
27 changes: 8 additions & 19 deletions tak.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,12 @@ cmd = [
]
runs = 10

# The gate: the same mise command line parsed by each framework, plus a binary that
# does everything except parse. The parse itself is the difference between the third
# number and the first — roughly two thirds of a small binary's instructions are the
# dynamic loader and libc starting up, and neither parser is responsible for those.
# The shadow benchmarks are deliberately *not* here. tak gates everything it finds in this
# file, and the mise shadow is a fixture that grows on purpose: teaching the derive command
# aliases added 91 names to it and 1.5% to the count, which the gate reported as a
# regression when it was the fixture getting bigger. A gate that fires for that teaches
# people to ignore it.
#
# `use -g node@20` rather than something exotic: it selects a subcommand, takes a
# global short, a switch and a value, which is what a real invocation looks like. The
# cost turns out to be near constant across shapes anyway, since there is no tree to
# build.
[bench.gate-baseline]
cmd = ["./target/release/parse-none", "use", "-g", "node@20"]
runs = 10

[bench.gate-usage]
cmd = ["./target/release/parse-usage", "use", "-g", "node@20"]
runs = 10

[bench.gate-clap]
cmd = ["./target/release/parse-clap", "use", "-g", "node@20"]
runs = 10
# They are measured and reported by `mise run perf:shadow`, which the pull-request workflow
# appends to its comment. What is worth watching there is the ratio between the two
# frameworks, which is a property of the parsers rather than of the fixture.
Comment on lines +30 to +38

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Document why gate = false is not used.

This comment explains why the shadow benchmarks are omitted, but it does not state that tak silently ignores unknown benchmark keys. Add that gate = false is not a supported opt-out. This prevents a future configuration change that appears to disable gating but has no effect.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tak.toml` around lines 30 - 38, Update the comment above the omitted shadow
benchmarks to state that tak silently ignores unknown benchmark keys, so gate =
false is not a supported opt-out and must not be used to disable gating.
Preserve the existing explanation of the shadow benchmark workflow and ratio
reporting.

69 changes: 69 additions & 0 deletions tasks/perf-shadow.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
#!/usr/bin/env bash
# The shadow comparison: what parsing a mise-sized command line costs each framework.
#
# Reported, never gated. The shadow is a fixture that grows on purpose — every property the
# derive learns to express adds to it — so comparing one commit's shadow against another's
# measures the fixture, not the parser. That is why these are not in `tak.toml`: tak gates
# everything it finds there, and a gate that fires when a fixture gets bigger teaches
# people to ignore it.
#
# What is worth watching here is the *ratio* between the two columns, which is a property of
# the parsers and holds steady while the fixture changes.
set -euo pipefail

out=${1:-/dev/stdout}

# Built somewhere else and moved into place at the end. The workflow calls this with
# `|| true`, because an informative table is not worth failing a run over — which means a
# failure part way through must not leave a truncated table behind for the comment to
# publish as though it were a comparison.
tmp=$(mktemp)
trap 'rm -f "$tmp" cachegrind.out.*' EXIT

# The two binaries take a repeat count from the environment, so differencing two runs of the
# *same* binary isolates the parse: N=1 minus N=0 is one cold parse in a fresh process.
# Differencing two different binaries would fold in whatever they each do before `main`.
instructions() {
local binary=$1 n=$2
PARSE_N="$n" valgrind --tool=cachegrind --cache-sim=no --branch-sim=no \
"./target/release/$binary" use -g node@20 2>&1 |
sed -n 's/.*I *refs: *//p' | tr -d ','
}

cold() {
local binary=$1 zero one
zero=$(instructions "$binary" 0)
one=$(instructions "$binary" 1)
echo $((one - zero))
}

if ! command -v valgrind >/dev/null 2>&1; then
echo "valgrind is not installed, so there are no instruction counts to report" >&2
exit 0
fi

usage_cold=$(cold parse-n)
clap_cold=$(cold parse-n-clap)

{
printf '### Shadow comparison\n\n'
# The backticks in this block are markdown, not command substitution, and the single
# quotes are what keeps them literal.
# shellcheck disable=SC2016
printf 'Parsing `mise use -g node@20` against a shadow of mise'"'"'s committed spec.\n'
printf 'Reported, not gated: the shadow grows as the derive learns to express more, so\n'
printf 'what to watch is the ratio rather than either column.\n\n'
printf '| | usage | clap | ratio |\n'
printf '|---|---:|---:|---:|\n'
printf '| instructions, cold parse | %s | %s | %sx |\n' \
"$(printf "%'d" "$usage_cold")" "$(printf "%'d" "$clap_cold")" \
"$((clap_cold / usage_cold))"
Comment on lines +58 to +60

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use a floating-point ratio and validate the denominator.

$((clap_cold / usage_cold)) uses integer division. A 1.9x result becomes 1x, and a 0.8x result becomes 0x. A zero usage_cold also aborts the report. Compute a formatted floating-point ratio and reject non-positive counts.

Suggested calculation
+ratio=$(
+  awk -v clap="$clap_cold" -v usage="$usage_cold" \
+    'BEGIN { if (usage <= 0) exit 1; printf "%.2f", clap / usage }'
+)
   printf '| instructions, cold parse | %s | %s | %sx |\n' \
     "$(printf "%'d" "$usage_cold")" "$(printf "%'d" "$clap_cold")" \
-    "$((clap_cold / usage_cold))"
+    "$ratio"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
printf '| instructions, cold parse | %s | %s | %sx |\n' \
"$(printf "%'d" "$usage_cold")" "$(printf "%'d" "$clap_cold")" \
"$((clap_cold / usage_cold))"
ratio=$(
awk -v clap="$clap_cold" -v usage="$usage_cold" \
'BEGIN { if (usage <= 0) exit 1; printf "%.2f", clap / usage }'
)
printf '| instructions, cold parse | %s | %s | %sx |\n' \
"$(printf "%'d" "$usage_cold")" "$(printf "%'d" "$clap_cold")" \
"$ratio"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tasks/perf-shadow.sh` around lines 51 - 53, Update the ratio calculation in
the instructions/cold parse output to validate that usage_cold is positive
before dividing, and report an error or otherwise reject invalid counts. Replace
the integer arithmetic in the printf with a formatted floating-point calculation
so fractional ratios such as 1.9x and 0.8x are preserved.

printf '\n'
# Wall clock in the same process, which is the only way to see µs: a whole-process
# measurement cannot resolve a 2µs parse.
printf '```\n'
./target/release/time-parse
printf '```\n'
} >"$tmp"

cat "$tmp" >"$out"