Go Perf Code Review

Go Performance Code Review

"Clear is better than clever." — Go Proverbs

"Premature pessimization is when you write code that is slower than it needs to be, usually by asking for unnecessary extra work, when equivalently complex code would be faster and should just naturally flow out of your fingers." — Herb Sutter, quoted in go-perfbook

Reviewing Go for performance is its own discipline, and it has the same shape as go-idiomatic-discipline's "clear AND correct" axis — two opposite ways for a reviewer to be wrong. This skill owns the review layer: what to flag, what to leave alone, and how to phrase it. Whether you should optimize a given path is go-perf-methodology (measure-first); each specific fix routes to its go-perf-* leaf (§5). The policy root — clarity is not negotiable for unproven speed — is go-idiomatic-discipline.

All Go below builds clean under gofmt, go vet, and go test on Go 1.26.

1. The Two Reviewer Failure Modes

A performance review fails in two opposite directions. Both feel like diligence. Both are wrong.

Axis 1 — letting premature pessimization through (too lax)

The reviewer waves through code that is "slower than it needs to be … when equivalently complex code would be faster" (go-perfbook, quoting Sutter). The recurring tells: s += x in a loop where strings.Builder is the same line count; an unbounded goroutine-per-request that will OOM under load; an obvious allocation inside a hot loop; []byte↔string churn on a hot path; an O(n²) scan where an O(n) of equal clarity exists. None of these is a readability tradeoff — the faster version is just as clear — so passing them is a free loss.

# WRONG reviewer move — approve it silently
"LGTM."                          # the for-loop s += chunk is quadratic; the fix costs no clarity

# RIGHT — name the free win, route the fix, keep it light
"Free win: this += in the loop is O(n²) copying — strings.Builder is linear and the same line
 count. See go-perf-strings-bytes-zerocopy."

Axis 2 — demanding premature optimization (too zealous)

The reviewer becomes the premature-optimization police: blocks a PR over a micro-opt on a cold path, demands a sync.Pool nobody benchmarked, asks for unsafe/SIMD to shave nanoseconds off code that runs once at startup, or rejects clear code because a clever version might be faster. Every Go Proverb pushes back — "Clear is better than clever" (Proverbs) — and optimization "generally comes at the cost of readability … optimized code is rarely simpler than the unoptimized version" (go-perfbook). Demanding that cost without a measured win is paying for nothing.

# WRONG reviewer move — mandate the unmeasured optimization
"Blocking: wrap this buffer in a sync.Pool to cut GC pressure."   # no profile, adds Reset complexity

# RIGHT — ask for the evidence, don't mandate the change
"Is this hot, and do we have a heap profile showing GC pressure? If so a pool may help
 (go-perf-sync-pool) — measure first (go-perf-methodology). If not, the plain alloc is clearer."

This skill names both axes and gives the decision rule and the wording. Depth on whether to optimize at all is go-perf-methodology; each concrete fix routes to its leaf (§5, §9).

2. The Meta-Rule: Flag Real Cost That Isn't a Clarity Tradeoff

The reviewer's decision for any line is a single test:

Flag it only when it is BOTH (a) a real, on-a-hot-path cost AND (b) fixable without sacrificing clarity. Everything else gets a question, not a change request.

This splits cleanly:

• Real cost + free fix (clarity equal or better) → flag it, suggest the fix. These are §3's free wins.
• Maybe a cost, fix would hurt clarity or is unproven → do not mandate a change; ask for a benchmark or profile (§7). Code Review Comments says exactly this about the most common micro-opt — value vs pointer receivers: "Don't choose a value receiver type for this reason without profiling first" (CodeReviewComments).
• Cold path → almost always leave it. Amdahl: "doubling speed in a routine consuming 5% of runtime yields only 2.5% total improvement" (go-perfbook). A micro-opt on startup code is noise you charged the author clarity for.

The point is symmetry: §3 is where you must speak up; §4 is where you must hold your tongue without evidence. The same diff line can land in either column depending only on which path it sits on and whether a measurement exists — so the reviewer's first question is rarely "is this slow?" but "is this hot, and do we have a number?"

3. The Free Wins — Flag These (real cost, no clarity tax)

These are the cases where the faster code is also the clearer or equal-clarity code, so there is no tradeoff to debate. Pass-through here is the axis-1 failure.

• += string building in a loop → strings.Builder (often quadratic copying vs linear). Same readability. Route: go-perf-strings-bytes-zerocopy.
• A known final size, no preallocation → make([]T, 0, n) / make(map[K]V, n) to skip repeated grow-and-copy / rehash. One extra argument, strictly clearer intent. Route: go-perf-slices, go-perf-maps.
• Unbounded goroutine-per-request/item → bound the fan-out (errgroup.SetLimit, a worker pool, a semaphore). This is a correctness-adjacent flag — unbounded concurrency is an OOM/overload bug, not a micro-opt. Route: go-perf-worker-pools-throughput.
• An obvious allocation in a measured-or-evidently-hot loop — boxing into any/interface{}, a per-iteration slice/map, []byte(s) round-trips — where moving it out of the loop or reusing a buffer (b = b[:0]) costs no clarity. Route: go-perf-escape-analysis, go-perf-slices.
• O(n²) where an equal-clarity O(n)/O(n log n) exists (a nested-loop "is x in this slice?" that a map[K]struct{} set replaces). Algorithmic, dwarfs micro-tuning; "data dominates" (go-perfbook). Route: go-perf-maps.
• fmt.Sprintf for trivial concatenation on a hot path → strconv.Append* / +. Route: go-perf-strings-bytes-zerocopy.

Note even these: if the path is provably cold (§6), they drop from "flag" to "optional nit" — say so.

4. The Premature-Optimization Traps — Don't Demand These

Asking for any of the following without a benchmark or profile in hand is the axis-2 failure. The right move is a question (§7), not a change request.

• "We should sync.Pool this." A pool adds Reset discipline, a cap-check-before-Put leak risk, and real complexity. It pays only under measured GC pressure. Don't mandate one nobody profiled — route the author to measure: go-perf-sync-pool + go-perf-methodology.
• unsafe / SIMD / hand-assembly for speed. These bypass bounds checks and the race detector and are unreadable. They are last-resort, post-measurement tools ("be dangerous" is go-perfbook's third tier, after algorithm and Go-level tweaks) (go-perfbook). Never the reviewer's opening ask. Route: go-perf-simd.
• Pointer-everything to "avoid copies." "Don't pass pointers as function arguments just to save a few bytes" (CodeReviewComments) — and a pointer can force a heap escape, making it slower. Don't demand it without escape analysis.
• A clever micro-rewrite that hurts clarity for an unproven delta. "Optimization is a form of refactoring … [that] improves some aspect of the performance" at a readability cost (go-perfbook) — so the bar is a measured goal you're missing, not a hunch.
• A map rewritten as a []struct (or vice versa) on vibes. For small N a slice can be both clearer and faster, but that's a measure-and-show call, not a mandate. Route: go-perf-maps.

5. The High-Signal Review Checklist

Skim a diff against this. Each row is a flag, its verdict, and the leaf that owns the fix the author should read.

The shape: the top rows (free wins) you raise; the bottom rows (unproven/clever) you question or drop. Same diff, opposite reviewer reflex.

6. When Performance Review Even Applies

Performance flags are not universal. Apply the lens by where the code lives:

• Hot path / library API / shared infrastructure — full §3 + §5 review. A library's allocation behavior is its callers' problem, and a hot path's cost is multiplied by call rate. "The fastest code is the code that's never run" (go-perfbook) — a library that allocates needlessly taxes everyone.
• One-off script, test, CLI startup, config parsing, cold init — review for clarity and correctness only. A strings.Builder in a build script's one-shot loop is a nit, not a blocker. Don't impose hot-path standards on code that runs once.
• Unknown hotness — don't assume. Ask "is this on a hot path?" rather than flag on suspicion; the answer decides whether §3 even applies. This is the reviewer's own version of "measure first" ([go-perf-methodology]).

A flag's weight scales with the path's heat. The same s += x is a real comment in a request handler and a shrug in a one-time migration.

7. How to Word Performance Feedback

The wording is the skill — the same observation can be a free win or premature-optimization policing depending on phrasing.

• For a free win (§3): state the cost, suggest the swap, keep it light. "This += in the loop is O(n²) copying — strings.Builder is the same line count and linear. Cheap to change." A free win is a suggestion, not a gate, unless it's an actual scaling bug (unbounded concurrency).
• For anything unproven (§4): request evidence, don't mandate a change. Ask "Is this on a hot path, and do we have a benchmark? If it's measurably costing us, here's the route (go-perf-sync-pool); if not, the simpler version is the right call." You are asking for a profile, not assigning a rewrite — Code Review Comments models this: profile before choosing the receiver type (CodeReviewComments).
• Never block a PR on a cold-path micro-opt. If it's not on a hot path and not a scaling bug, it's at most a non-blocking nit — label it so. Blocking here is the axis-2 failure made visible to the author.
• Don't accept "it's faster" on faith either. One run is inside the noise; ask for n≥10 and benchstat before the claim lands, just as you'd ask before the change (§5 last row → go-perf-methodology).
• Separate the scaling bug from the micro-opt in tone. Unbounded fan-out is "this will fall over in prod" (a real blocker); a missing slice prealloc is "free win, optional." Conflating them makes you the premature-optimization police on the things that are premature.

8. Don't

• Don't wave through a free win — +=-in-a-loop, unbounded fan-out, a known-size slice with no prealloc are real costs at zero clarity tax (§3).
• Don't mandate a sync.Pool, unsafe, SIMD, or a clever micro-rewrite without a benchmark/profile — that's demanding the readability cost of optimization with no measured buyer (§4).
• Don't block a PR over a cold-path micro-opt — Amdahl says a 2× on 5% of runtime is 2.5%; route it to a nit or drop it (§2, §6).
• Don't flag on suspicion of hotness — ask whether the path is hot before applying the perf lens (§6).
• Don't treat "it's faster" as proven from one run — require n≥10 + benchstat for the claim (§5, §7).
• Don't sacrifice clarity for unproven speed, or accept a PR that does — "Clear is better than clever" is the tie-breaker (Proverbs).
• Don't conflate a scaling bug with a micro-opt — unbounded concurrency is a blocker; a missing prealloc is a suggestion (§7).
• Don't apply hot-path standards to a one-off script, test, or cold init — review those for clarity and correctness only; the perf lens is for hot paths and library APIs (§6).

9. Routing to Related Skills

The policy root and the should-I gate:

• go-idiomatic-discipline — the dual-axis policy root ("clear AND correct"); this skill is its performance-review mirror.
• go-perf-methodology — whether to optimize at all: measure-first, profile/benchmark, benchstat n≥10, Amdahl, the cold-path guard. The "ask for evidence" in §4/§7 cashes out here.

The specific fixes each flag routes to (go-perf-* leaves):

• go-perf-worker-pools-throughput — bounding an unbounded goroutine-per-request fan-out.
• go-perf-strings-bytes-zerocopy — strings.Builder vs +=, strconv.Append* vs fmt.Sprintf, []byte↔string cost.
• go-perf-slices / go-perf-maps — preallocation, buffer reuse, set-instead-of-O(n²), slice-vs-map for small N.
• go-perf-escape-analysis — does this allocation/pointer actually escape; -gcflags=-m before a pointer rewrite.
• go-perf-sync-pool — when a pool is justified (measured GC pressure) and its hazards.
• go-perf-simd — why unsafe/SIMD/assembly is a last resort, not a review opener.
• go-perf-goroutines-scheduler — goroutine/stack cost and GOMAXPROCS behind the fan-out flag.

10. Reference Files

Wrong/right review-behavior contrasts — the two failure modes in action (blocking a cold-path nit, passing an unbounded goroutine, demanding an unmeasured pool), each with the better reviewer move and citations:

references/common-mistakes.md

Source provenance for every claim in this skill:

references/sources.yaml