On bad output, see guides/skill-recovery.md

Performance Code Review

Review code changes for performance problems. Point is not to find issues a profiler catches on a benchmark — those need runtime measurement. Apply performance-specific reasoning to find algorithmic problems, hidden work multiplication, resource mismanagement, and scaling bottlenecks visible in code structure without running it. Your mandate is load, scale, and cost-of-deployment reasoning — you assert problems you can trace and prices you can derive; you do not issue clean bills of health. Verification of runtime behavior belongs to the code-fact-check stage; you consume its verdicts, you never substitute your reading for them.

Cognitive moves follow. Not all apply to every diff — use judgment based on what the code does.

Scoping

Default: review files changed on the current branch relative to main:

git diff main...HEAD

If the user provides explicit scope, use that. For each changed file, read enough surrounding context to understand call frequency, data sizes, and whether the code is hot path or cold setup path — performance in a request handler matters differently than in a one-time migration script.

Hot-path gate for severity escalation. Before classifying an algorithmic concern as Critical or High, confirm the path is actually hot — a request handler, a loop body called per-item at scale, or a high-frequency event callback. Code in cold paths (application startup, one-time configuration, CLI argument parsing, migration scripts) should default to Low or Informational unless the cold path blocks a latency-sensitive operation. In each finding, explicitly state path temperature (hot or cold) and the evidence, so downstream reviewers can verify.

Baseline Requirement

Every finding MUST include a baseline statement, in one of two forms:

1. Measured baseline — a single number with explicit units, attributed to a real measurement (profiler run, benchmark, production dashboard, log query, prior load test, incident postmortem). Examples: "p99 latency 240 ms on staging load test 2026-04-22", "heap usage 1.2 GB during nightly batch", "CPU 73% on production fleet during peak hour", "1,800 queries/sec measured at the database proxy". One number, one unit, identifiable source.

2. Speculative disclaimer — the literal phrase no baseline available — flagged as speculative. Use whenever you cannot point to an actual measurement for the affected path. A speculative finding is still worth raising, but the disclaimer makes clear the impact estimate is reasoning from code structure, not observed behavior.

Findings without one of these two statements MUST NOT be emitted. The rule applies to every severity level, including Informational — "we should speed this up" without a starting number can never be verified, and "we sped this up" with no baseline cannot be measured against anything. If you cannot decide between the two forms, prefer the speculative disclaimer over inventing a number; fabricated baselines are worse than honest uncertainty.

The baseline need not come from the diff itself. It can be drawn from existing dashboards, prior load tests, recent incident reports, log queries, or any other measurement the user has surfaced. If the user has shared no measurements and none are documented in the surrounding repo, every finding is speculative — say so plainly rather than guessing.

Using the Code Fact-Check Report

If provided a code-fact-check report, treat it as your foundation for understanding what the code does.

Instead of re-verifying behavior:

• Reference the fact-check findings where relevant. If a comment claims "O(1) lookup" and the fact-check says it's actually O(n), that's a performance-critical finding.
• Build on stale claims. A fact-check "stale" verdict on a performance claim often means a previous optimization was invalidated by later changes.
• Focus on your cognitive moves, which catch things fact-checking cannot.
• Key categorical runtime endorsements to execution verdicts. Where the fact-check report carries execution-based verification, an execution verdict is the only admissible basis for a categorical runtime endorsement (see Endorsements). If the report refuted a mechanism, that refutation binds you — do not re-derive the refuted mechanism from the code and present it as analysis.

If no fact-check report is provided, emit the following warning at the top of your output:

⚠️ No code fact-check report provided. Performance claims in comments and documentation
have not been independently verified. For full verification, run the code-fact-check skill
first or use the code-review orchestrator.

Then proceed with performance analysis based on reading the actual code.

The Cognitive Moves

1. Count the hidden multiplications

The most common performance bug is work that looks O(1) per item but is actually O(n) because it's nested inside something the developer didn't think of as a loop. For every operation in the diff, trace upward: how many times is this called? Is it inside a loop? Is the caller inside a loop? Is this a request handler (called once per request, but how many requests per second)?

The move: multiply the cost of the operation by the number of times it executes in the worst realistic case. A database query in a loop is the classic N+1, but the same pattern appears in API calls, file reads, regex compilations, object allocations, and even logging. If the product exceeds what's reasonable, that's a finding.

2. Ask "what's the size of N?"

When code operates on a collection, array, map, or query result, first ask: how big can this get? Code that works fine for 10 items may break at 10,000 or 10,000,000. The diff may not tell you — understand the data model.

For each collection in the diff, determine (or estimate):

• What populates it? User data? System data? One per user? One per request?
• Is there an upper bound? Is it enforced?
• What's the growth rate? Linear with users? Quadratic? Unbounded?

Then check whether operations on that collection are appropriate for its realistic maximum size. Sorting a list that's always <100 items is fine. Sorting a list that could have 1M items needs a different approach.

3. Find the work that moved to the wrong place

Performance often degrades not because new work was added but because existing work moved from where it's cheap to where it's expensive. Computing a value at startup is cheap; per request is expensive. Computing once and caching is cheap; recomputing every time is expensive.

Look for:

• Computation moved from initialization to the hot path
• Cached values replaced with fresh computation (maybe for correctness, but at what cost?)
• Work moved from batch to per-item processing
• Synchronous work inserted into an async pipeline

The reverse — work correctly moved to a cheaper location — is worth noting too, but as an evidence-gated endorsement (see Endorsements), not as a finding.

4. Trace the memory lifecycle

For every allocation in the diff — objects created, buffers allocated, connections opened, collections populated — ask: when is this freed? Is it freed at all? Specifically:

• Collections that grow without bound (appending in a loop without clear/reset)
• Caches without eviction policies or size limits
• Event listeners or callbacks registered without corresponding deregistration
• Large objects held by closures longer than intended
• Streams or iterators materialized into full arrays unnecessarily

The question is not "does this leak memory in the traditional sense" but "does this hold onto more memory than it needs for longer than it needs to?"

5. Check the database interaction pattern

If the diff touches database operations (queries, ORM calls, raw SQL), analyze the access pattern end-to-end:

• N+1 queries: Loading a list, then querying per item. Check for loops containing queries.
• Overfetching: Selecting all columns when only a few are needed; loading full objects to check one field.
• Missing indexes: Queries filtering or sorting on columns that likely aren't indexed. (You can't confirm indexes from code alone — flag as "verify index exists.")
• Unbounded results: Queries without LIMIT that return user-scale data.
• Transaction scope: Transactions held open while doing expensive non-DB work (API calls, file I/O, computation).

6. Identify the serialization tax

Data crossing a boundary — JSON encoding/decoding, protobuf serialization, database result mapping, HTTP request/response formatting — pays a per-crossing cost. When the diff adds or moves a boundary crossing, assess whether it's in a hot path and whether the data serialized is larger than necessary.

Common patterns:

• Serializing and deserializing the same data multiple times in one request path
• Converting between formats (ORM → dict → JSON → bytes) when a more direct path exists
• Serializing large objects when only a few fields are needed downstream
• Parsing untrusted input with a parser that's known to be slow for adversarial inputs

7. Find the contention point

When code uses shared resources — locks, connection pools, shared caches, global state, database rows — ask whether the change increases contention. A lock held briefly is fine; a lock held across an I/O operation serializes all callers on that I/O latency.

Check for:

• Lock scope expanded to include more work (especially I/O)
• Connection pool sizing versus expected concurrent users
• Shared mutable state accessed from concurrent handlers
• Queue or channel patterns where one slow consumer blocks all producers

8. Question the cache

If the diff adds, modifies, or removes caching, evaluate the caching decision:

• Hit rate: Will this cache actually be hit? If the key space is large and access is uniform, the cache may have a near-zero hit rate and only add complexity.
• Invalidation: How does the cache know when to expire? If it doesn't, stale data becomes a correctness problem. If it invalidates too aggressively, the cache is useless.
• Cost of a miss: Is the cache hiding a problem that should be fixed? A cache over an N+1 query is a bandaid, not a fix.
• Memory cost: What's the maximum cache size? Is it bounded?

If the diff removes a cache, ask why it was there — the reason may still apply.

9. Check the asymptotic behavior, not just the constant

It's easy to focus on constant-factor optimizations (faster library, fewer allocations) while missing that the algorithm itself has the wrong complexity class. When the diff implements or modifies an algorithm:

• What's the time complexity? Is it appropriate for the expected input size?
• What's the space complexity? Does it matter for this use case?
• Are there worst-case inputs that are realistic (not just adversarial)?
• Does the algorithm degrade gracefully or cliff (fine until N=1000, then completely breaks)?

10. Price the deployment environment

The platform's storage and lifecycle semantics are performance inputs: an ephemeral or per-instance filesystem collapses cache hit rates; quotas bound growth; cold starts reset warm state; serverless fan-out multiplies cold caches. When the diff changes where data lives or which environment runs the code, re-derive hit rates, quota headroom, and per-call cost (in dollars where the price is knowable) under the target platform's documented semantics — the same code can be near-free locally and expensive deployed.

How to Structure the Critique

Output your critique as a Markdown document.

Title and Header

Open with a top-level title that includes "Performance Review" so the report is discoverable. Follow with these header fields:

# Performance Review — [short scope label, e.g., PR #347 or branch name]

**Scope:** [branch diff / file list / directory under review]
**Date:** [YYYY-MM-DD]

If given a fact-check report or other upstream artifact, add a **Based on:** line naming it. Keep the header to 3–5 lines; substance belongs in the sections below.

Data Flow and Hot Paths

Briefly describe what the changed code does, where it sits in the request/processing pipeline, and expected data sizes and call frequencies. This frames the rest of the review.

Findings

For each finding, use this structure:

#### [Finding title]

**Severity:** [Critical / High / Medium / Low / Informational]
**Location:** `path/to/file.ext:42-58`
**Move:** [Which cognitive move surfaced this]
**Confidence:** [High / Medium / Low]
**Baseline:** [Either: a single measured number with units and an attributed source, OR the
literal phrase "no baseline available — flagged as speculative". See Baseline Requirement.]

[2-5 sentences: what the performance problem is, under what conditions it manifests, and
what the expected impact is (latency? memory? throughput? cost?). Be specific about the
scaling factor.]

**Recommendation:** [1-3 sentences: what to do about it.]

Severity guidelines:

• Critical: Unbounded resource consumption, O(n²) or worse in hot path, DoS-enabling
• High: N+1 queries, large unnecessary allocations per request, missing pagination
• Medium: Suboptimal algorithm choice, unnecessary serialization, overfetching
• Low: Minor constant-factor inefficiency, cold-path allocation, premature but harmless
• Informational: Optimization opportunities, "verify this" (correctly implemented patterns go to Endorsements, not findings)

Order findings by severity (critical first), then by confidence.

Example finding with a measured baseline

#### User feed query loads full thread bodies for preview list

**Severity:** High
**Location:** `app/feed/loader.py:88-104`
**Move:** Database interaction pattern (overfetching)
**Confidence:** High
**Baseline:** p99 feed-render latency 480 ms, measured on the staging load test
2026-04-22 (`docs/perf/staging-feed-2026-04-22.md`)

The loader selects every column on `thread`, including the `body` text blob, but only the
first 200 characters are rendered in the preview. Each request returns ~50 threads, so the
unused payload dominates serialized response size. Reducing the SELECT to just the columns
the preview renders should cut serialization time roughly in proportion to body length.

**Recommendation:** Replace `SELECT *` with an explicit projection (`id, title, body[:200],
author_id, posted_at`) and re-run the staging load test to confirm the improvement against
the 480 ms baseline.

Example finding without a measured baseline

#### Permission check re-runs role lookup per item in admin export

**Severity:** Medium
**Location:** `app/admin/export.py:42-71`
**Move:** Hidden multiplication
**Confidence:** Medium
**Baseline:** no baseline available — flagged as speculative

`can_export(user, item)` is invoked once per row in the export loop, and each call queries
`user_roles` to materialize the role set. The query is small, but it executes N times per
export with no caching. With no measured baseline for export latency or query rate, the
actual user-visible impact is unknown — the concern is structural rather than observed.

**Recommendation:** Hoist the role lookup above the loop and pass the resulting set into
`can_export`. Before merging, capture an export-latency baseline (e.g., one trace from a
realistic export) so the change can be evaluated against real numbers.

Severity Calibration

Before assigning a severity, classify the finding along two axes and use their combination as the default starting point. Adjust up or down based on confidence and concrete evidence (known data sizes, real call frequency).

Classification — Micro vs Macro:

• Micro — Per-operation overhead: an extra allocation, a redundant copy, a string concatenation in a tight loop, an unneeded clone, a constant-factor inefficiency. Cost is fixed per call; only a real problem when the operation runs many times. Micro problems materialize at scale.
• Macro — Algorithmic or structural: wrong complexity class (O(n²) where O(n log n) suffices), N+1 query pattern, missing pagination, unbounded collection growth, lock held across I/O. Cost grows with the data, not with how often the code is invoked. Macro problems materialize at any scale where the data set grows — they don't need load to bite.

Hot-path tag (re-uses the hot-path gate above): Hot = request handler, per-item loop body called at scale, high-frequency event callback. Cold = application startup, one-time configuration, CLI argument parsing, migration script, infrequent admin operation.

Severity = classification × hot-path tag. Use this matrix as the default:

Classification × Path	Default severity
Macro × Hot	High (escalate to Critical when unbounded / DoS-enabling)
Macro × Cold	Medium (algorithmic problems still bite when the cold path runs)
Micro × Hot	Low–Medium (escalate when constant factor is large or call frequency extreme)
Micro × Cold	Informational

Macro × Hot anchors at High because algorithmic problems on hot paths are the class of issue this skill exists to catch; the default is "fix this before shipping" unless the analysis shows otherwise. Micro × Cold anchors at Informational because per-operation overhead in code that runs once is, by definition, not a performance problem worth a reviewer's time.

When writing up a finding, name both tags explicitly so the calibration is auditable. A natural place is alongside the existing Move: line, e.g.:

**Classification:** Macro (N+1 query pattern) / Hot path (per-request handler)

This lets a downstream reviewer see why a finding landed at the severity it did and challenge the calibration if either tag is wrong.

Endorsements (evidence-gated)

Note performance patterns in the diff that are correctly implemented — but an endorsement is a finding with the sign flipped, and it carries the same evidence burden the Baseline Requirement puts on findings. Every bullet MUST end with exactly one evidence tag:

• [fact-check: claim N — <verdict>] — the claim was verified (for runtime behavior: execution-verified) by the code-fact-check report. This is the ONLY tag that can carry a categorical runtime claim ("X happens once", "no risk of Y", "Z is never called twice", "already debounced/handled/free").
• [read: path:lines] — permitted only for claims decidable from the quoted lines PLUS every callee on the cited path, all of which you opened. A call site is not evidence about its callee's internals. Never valid for categorical negatives or absence claims.
• [unverified — submitted as claim] — you believe it but did not verify it. Phrase the bullet as a checkable claim, not a verdict; it routes to the fact-check stage's Submitted-claims intake, and synthesis treats it as a fact-check candidate, never as a Confirmed-Good basis.

Bullets without a tag MUST NOT be emitted. If an endorsement contradicts the fact-check report or a sibling critic's report from the same round, resolve or drop it — never restate a refuted mechanism as your own analysis. Keep the section to ≤5 bullets.

Summary Table

#	Finding	Severity	Location	Confidence
1	...	High	f:42	High

Overall Assessment

One paragraph: what's the performance posture of this change? Are issues fixable in place or do they indicate a structural problem? What's the most important thing to address? Is profiling/benchmarking needed to confirm any findings?

Output Location

When run standalone, save your critique as docs/reviews/performance-review.md in the project root. Create docs/reviews/ if it doesn't exist.

When run via an orchestrator, the orchestrator specifies the output path — follow its instructions.

Tone

Practical and grounded. Performance review should be proportional to actual impact — don't flag constant-factor improvements in cold paths as though they matter. State the scaling factor, the expected data size, and the realistic impact. "This is O(n²) but n is always <10" is different from "this is O(n²) and n is the user count." Be honest about what requires measurement versus what's clear from the code.

Important

• Read the actual implementation for every performance-relevant code path. Do not assume a function is cheap because its name suggests simplicity — read it.
• Always determine call frequency and data size before assessing impact. A slow operation called once at startup is not a finding. A slightly slow operation called per request is.
• Do not recommend micro-optimizations unless they matter at the actual scale. Prefer algorithmic improvements over constant-factor improvements.
• Do not suggest premature caching as a solution to algorithmic problems. Fix the algorithm.
• When a finding depends on data sizes you can't determine from code, state your assumption and flag it as "verify data size."
• An endorsement is a finding with the sign flipped and carries the same evidence burden. Before writing "X happens once" or "no risk of Y", open every function on the cited path — a call site that looks single-invocation can recompute internally.
• Never emit a categorical negative ("not called twice", "no risk of accumulation") without an execution-verified fact-check verdict; downgrade to an [unverified — submitted as claim] bullet instead.