Repository navigation
[bench]: Add hot-path benchmarks and an A/B benchmark driver - #218
Conversation
|
|
pseudomuto
left a comment
pseudomuto
left a comment
There was a problem hiding this comment.
Hello again @rainman456, thanks for PR!
Having benchmarks at all is a big step up 💪 Most of my comments are nits and other minor concerns. The larger suggestion is about bench/run-benchmarks.sh. In sum, I think we should drop it (well, replace it really).
The script's idea is solid 👌 IIUC, we compare two revisions in a worktree, alternate rounds so noise spreads evenly, and let benchstat do the stats. I have a few concerns about the other parts of the script that I think should be addressed. I commented inline in the file, but to summarize:
- Linux only: /proc/cpuinfo, and nproc aren't on stock macOS), so on a Mac it prints errors instead of attesting anything
- Pinning GOMAXPROCS to nproc is already Go's default, so it doesn't change the run (and requires nproc to be available).
- IMO, the profiling pass (CPU, heap, block and mutex for each package) is a separate job from comparing two revisions, and we have config-gated pprof routes now (#220) for real profiling (thanks to you).
- It writes untracked artifacts into the repo root and runs an unpinned benchstat@latest.
I think we can simplify this into a short mise task, keeping it similar to the other dev commands and pinning the tooling.
If we drop something like this into mise-tasks/test/bench, we would be able to run it via mise run test:bench [ARGS] (we'd need to remove the test:bench task from mise.toml).
Sample bench:test task
I adapted this from your script, but maybe dropped something meaningful (please double check 😄).
#!/usr/bin/env bash
#MISE description="Run benchmarks, optionally comparing against another revision"
#USAGE flag "-b --base <ref>" help="Compare the working tree against this revision with benchstat"
#USAGE flag "-c --count <count>" default="10"
#USAGE flag "-t --time <time>" default="1s"
#USAGE flag "-p --packages <packages>" default="./..."
set -euo pipefail
bench() {
go test -run '^$' -bench . -benchmem -count="$1" -benchtime="$usage_time" $usage_packages
}
if [[ -z "${usage_base:-}" ]]; then
bench "$usage_count"
exit
fi
work="$(mktemp -d)"
git worktree add --detach "$work/base" "$usage_base" >/dev/null
trap 'git worktree remove --force "$work/base"; rm -rf "$work"' EXIT
# Alternate one round at a time so noise lands evenly on both sides.
for i in $(seq "$usage_count"); do
echo "round $i/$usage_count" >&2
(cd "$work/base" && bench 1) >>"$work/base.txt"
bench 1 >>"$work/cand.txt"
done
# TODO: optionally tee to a gitignored file/dir in root for further analysis
benchstat "$work/base.txt" "$work/cand.txt"Do you think this makes sense? Definitely open to discussing any/all of it more.
|
Thanks for the thorough pass. All ten comments addressed in
One note on the lockfile: Fork CI (lint, test, build) is green on the pushed head |
|
This is looking good 👍. Do you have any thoughts on the mise task, etc. I mentioned in the review notes? |
|
Agreed, a mise task is a better home for this. 47f84ed replaces bench/run-benchmarks.sh with mise-tasks/test/bench, so I used your sample as-is, plus two small keeps from the script:
Agree on dropping the profiling pass; #220 covers real profiling now. The rest of the header (kernel, CPU model, Go version) goes with it, the SHAs are what a captured run needs. Your tee TODO stays, along with the gitignore entries for when someone implements it. Fork CI (lint, test, build) green on 47f84ed. |
|
@rainman456 I think it needs a rebase since #234 was merged. Otherwise, this looks great! Once the rebase is done, we'll get this in right away. Thanks for all your work in seeing this through ✊ |
The repo has no benchmarks, so performance claims have nothing to measure against. This adds benchmarks for every stage of the request path: namespace extraction and translation (protoutil), rule matching and the routing decision including the decision metric (router), and one full unary round trip through the gateway shape - hybrid codec, metrics interceptor, PeekInterceptor, Director, per-upstream proxy - against a no-proxy baseline over the same bufconn upstream. Whole-path allocations are the headline number: a proxied unary call allocates 400 times against 160 for a direct one. bench/run-benchmarks.sh drives A/B comparison of two revisions: it alternates base and candidate one round at a time, pins GOMAXPROCS, and reports benchstat plus pprof tops. The header it prints carries the commit SHAs, CPU model, and Go version, so a captured run is self-attesting. CI compiles the benchmarks but does not run them; mise test:bench runs them on demand. The test helpers dialBufconn, serve, prepared, newTestReporter, and newRelayWith take testing.TB now, so benchmarks reuse them instead of duplicating the bufconn harness.
Share the StartWorkflowExecution bench request through internal/testutils so the extractor, translator, and relay benchmarks describe one payload. Fold the standalone mux benchmark into BenchmarkDirector_Resolve, which covers the same shapes through the real Resolve path, and drop the cold translator benchmark and the warm-service assertion the fx module already guards.
Use getconf and sysctl instead of nproc and /proc/cpuinfo so the header works on macOS, and rely on Go's default GOMAXPROCS like every other run. benchstat now comes from the mise tools section instead of go run @latest, and the artifacts the driver writes are gitignored.
bench/run-benchmarks.sh grew a profiling pass and a header block that duplicated what config-gated pprof routes and benchstat already do. The mise task keeps the useful core: alternate A/B rounds against a base revision in a worktree, then compare with the pinned benchstat. Task arguments cover count, benchtime, and packages; running without --base just benchmarks the working tree, replacing the old test:bench task.
The module path changed to go.temporal.io/proxy in temporalio#234 while this branch was open, so the benchmark files introduced here still imported the old path and broke typechecking on the merge. Rewritten to the new path; the imports keep their group order, which gci now reads as the localmodule section.
47f84ed to
8c68cff
Compare
|
Rebased, thanks for the nudge. #234 moved the module path to The branch is now the four bench commits plus that one, on top of |
pseudomuto
left a comment
pseudomuto
left a comment
There was a problem hiding this comment.
Thanks for this @rainman456!
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
4 participants
The repo has no benchmarks, so performance claims have nothing to measure
against. This adds benchmarks for every stage of the request path:
router,
interceptor, PeekInterceptor, Director, per-upstream proxy) against a
no-proxy baseline over the same bufconn upstream.
The whole-path pair is the headline. A proxied unary call allocates 400 times
against 160 for a direct one, so any change to the forward path now has a
number to answer to.
Here is what a 10 round run of these benchmarks reports on a shared 2 vCPU
host. Allocs/op and B/op are deterministic; timing on shared hardware moves a
few percent between rounds.
mise run test:benchbenchmarks the working tree, and with-b <ref>itcompares against that revision: rounds alternate between the working tree and
a detached worktree of the base so noise lands evenly on both sides, then the
benchstat comparison is printed. Count, benchtime, and packages are task
flags.
The test helpers dialBufconn, serve, prepared, newTestReporter, and
newRelayWith take testing.TB now so benchmarks can reuse them instead of
duplicating the bufconn harness. CI compiles the benchmarks but does not run
them; mise run test:bench runs them on demand.