performance-review

Files (4)

• SKILL.md
• DETECTION-PLAYBOOK.md · references/
• QUALITY-FILTER.md · references/
• REVIEW-VOICE.md · references/

SKILL.md (first lines)

# Performance Review

A specialised lens on top of [`pr-review`](https://github.com/priyank766/OpenSource-SKILL/blob/main/skills/pr-review/SKILL.md). Run that procedure
first; this one replaces the line-by-line audit with a measurement discipline.

**Read first:** [`DETECTION-PLAYBOOK.md`](references/DETECTION-PLAYBOOK.md) ·
[`QUALITY-FILTER.md`](references/QUALITY-FILTER.md) ·
[`REVIEW-VOICE.md`](references/REVIEW-VOICE.md)

> **Where the time goes.** Finding the flaw is the review. The playbook above is the
> general flaw-hunting procedure; this file is the domain-specific lens on top of it.
> Scoring and writing the comment are the last ten minutes, not the work.

**Severity floor: 8.0.** Comment budget: one per ~200 changed lines, max 5.

---

## The governing rule: no measurement, no comment

An unmeasured performance suggestion cannot score above 6.0, because you cannot claim
Correctness on a number you did not produce. Under the shared filter that is a silent
reject. This is not a stylistic preference — it is the spine of the whole skill.

| Claim | Evidence required | Without it |
| :--- | :--- | :--- |
| "This is slow" | base vs head timing, n runs, variance | reject |
| "This allocates too much" | peak RSS or heap profile, both sides | reject |
| "This would be faster as X" | X implemented and benchmarked | reject |
| "This is O(n²) on a hot path" | the loop nesting in the diff + a call-graph path | **arguable** |
| "This cache/list grows without bound" | no eviction, no cap, unbounded key space | **arguable** |

Two exceptions, and only two. **Complexity-class regressions** are arguable from the diff
because the failure mode is structural: it does not matter what the constant factor is,
the curve crosses. **Unbounded resource growth** is arguable because there is no input
size at which it stops. Everything else — every constant-factor claim, every "this could
be faster" — is a hypothesis until you have run it twice.

The corollary matters as much: if you *did* measure and the win is under ~10% with
overlapping variance, you also have nothing. Report it as "no measurable difference" and
drop the comment.

---

## Why this exists

Performance is where AI review is at its worst. The pattern-match is easy (`for` loop
plus a function call), the confidence is high, and the cost lands on the maintainer, who
must either relitigate an unmeasured suggestion or look unresponsive. A project that has
absorbed twenty "consider using a list comprehension here" comments stops reading
performance feedback entirely — including the N+1 that matters.

### What each role gets out of it

| Role | Time this gives back |
| :--- | :--- |
| **Maintainer** | You stop relitigating unmeasured micro-opt. Anything that reaches you carries base/head numbers, so the discussion is about the trade-off, not about whether an effect exists. |
| **Contributor** | You find out your change regressed a hot path before a maintainer does, and you arrive with the benchmark already run — which is usually what unblocks the merge. |
| **Project owner** | A written standard for what counts as a performance claim in this repo, so "make it faster" PRs come with evidence by default. |

---

## Procedure

```mermaid
flowchart TD
    A[Start] --> B[1. Isolate the diff]
    B --> C[2. Is the changed code on a hot path?]
    C -->|"startup or cold path"| Z[Stop: not a performance review]
    C -->|"per request or per item"| D[3. Classify: complexity, resource, or constant factor]
    D -->|"complexity class or unbounded growth"| E[4a. Argue from the diff]
    D -->|"constant factor"| F[4b. Benchmark base and head]
    F -->|"no measurement possible"| G[Drop the finding]
    F -->|"overlapping variance"| G
    F -->|"clear separation"| H[5. Score against the calibration table]
    E --> H
    H -->|"below 8.0"| G
    H -->|"8.0 or above"| I[6. Write it up with numbers and a patch]
```

### 1. Isolate the diff

```bash
git fetch upstream 'refs/pull/<PR>/head:pr-<PR>'
git diff upstream/main...pr-<PR> --stat
git diff upstream/main...pr-<PR> -- '*.sql' '*.py' '*.go' '*.ts' '*.rs'
```

Note the base SHA. You will need to check it out and run the same benchmark on it.

```bash
git merge-base upstream/main pr-<PR>
```

### 2. Establish whether the code is hot at all

This step kills most candidate findings, and it should. Startup code that runs once,
CLI argument parsing, migration scripts, and test helpers are not performance surface no
matter what the loop looks like.

```bash
# who calls the changed function, and from where
grep -rn "process_batch" --include='*.py' . | grep -v test

# does it sit under a request handler / job loop / event handler?
grep -rn -B3 "process_batch(" src/ | grep -E "route|handler|@app|consume|for .* in "

# is it import-time work? (module level, decorators, class bodies)
git diff upstream/main...pr-<PR> | grep -E '^\+[^ \t]' | grep -v '^\+\(def\|class\|import\|from\|#\)'
```

If the path is cold, stop. If you cannot establish that it is hot, say so in the comment
and downgrade the claim — "if `process_batch` runs per request this matters; if it's only
the nightly job, ignore me."

### 3. Classify the finding

- **Complexity class** — nesting, membership tests, repeated scans. Arguable from the diff.
- **Resource growth** — caches, buffers, accumulating collections, listeners. Arguable.
- **Constant factor** — everything else. Requires measurement.

From priyank766/OpenSource-SKILL, Apache-2.0.

More from priyank766/OpenSource-SKILL