N+1 Queries, Unbounded Memory, and Hot Loops: Spotting Performance Defects in Review

Why performance defects have to be caught by reading, not measuring

Performance defects have a property that makes review uniquely valuable: they are invisible at development scale and unavoidable at production scale. A query inside a loop is instant with the ten rows in your fixture and catastrophic with the ten thousand rows a real tenant has. Nothing in local testing, CI, or a code health dashboard distinguishes the two — but the shape is right there in the diff, and the shape is what you review.

The other reason to catch them early: performance defects are usually architectural in miniature. Fixing an N+1 after release often means changing a data access pattern, a serialisation format, or an API contract, which is a much bigger change than adding a JOIN FETCH during review.

What it costs in production

• Latency that scales with the customer's success. Your best customer has the most data and therefore the worst experience. The complaints arrive from exactly the accounts you cannot afford to lose.
• Out-of-memory kills. Loading a whole file or result set into memory works until a file is large, then the process is killed by the OOM killer mid-write, leaving partial state and no stack trace.
• Connection pool exhaustion. An N+1 turns one request into 500 queries. At fifty concurrent requests the pool is empty and unrelated endpoints start timing out — the outage looks like a database problem, not a code problem.
• Cost that compounds silently. Extra queries, extra egress, extra instances. Nothing breaks, so nothing gets prioritised, and the bill grows quarterly.
• Lock contention and thrash. Work that is technically parallel spends its time coordinating; adding workers makes it slower, which is deeply counterintuitive during an incident.
• Cascading timeouts. A slow endpoint holds threads, which fills the queue, which trips upstream timeouts and retries, which multiplies the load that caused it.

How to spot it in review

Read for the number of times something happens and the amount of data resident at once. Almost every performance defect is one of those two.

Loops that contain I/O. The highest-yield pattern in application code. For each loop in the diff, ask what is inside it: a query, an HTTP call, a file read, a cache miss, a log flush, or a lock acquisition. A single call inside a loop over user data is an N+1.

• ORM lazy loading is the sneakiest version — order.getItems() looks like a field access and is a query. In the diff you see a for loop with no visible I/O at all.
• GraphQL resolvers and serialiser to_representation hooks have the same property: the loop is in the framework, not in the diff.

Unbounded reads. readlines(), read(), findAll(), ToList() on an unfiltered set, SELECT * without a LIMIT, buffering an entire response body. Ask: what is the largest this can be in production, not in the test?

Unbounded growth. A cache, map, or list that is added to and never evicted or trimmed. A dedupe table with no retention. A retry queue with no cap. These are memory leaks with extra steps.

Work repeated per element that could be done once. Compiling a regex, building a formatter, opening a connection, or re-fetching configuration inside the loop body.

Quadratic shapes. A nested loop over two collections that both grow (for a in listA: for b in listB: if a.id == b.id) — build a map instead. String concatenation in a loop in languages where strings are immutable. list.remove(x) or contains inside a loop is O(n) per call.

Chatty or contended coordination. Per-item access to a shared/proxied structure across processes, a lock taken inside the hot loop, or a synchronous cross-service call per item.

Fixing it without breaking something else

• Eager fetching can explode the result set. JOIN FETCH on two collections produces a cartesian product; the fix for one N+1 can return ten times the rows. Batch loading or a second query is often better than one big join.
• Batching changes failure granularity. One failed row can now fail 500 rows. Partial-failure handling, retry semantics, and idempotency all need to be revisited when you batch.
• Streaming changes transaction and connection lifetime. A streamed result keeps a cursor and a connection open for the duration of processing, which can exhaust the pool faster than the eager read you replaced.
• Caching introduces staleness and a new failure mode. Every cache needs an invalidation story, a bound, and an answer for what happens on a miss storm. A cache added to fix a slow query hides the slow query rather than removing it.
• Adding an index slows writes and needs to be built without locking the table. Check whether an existing composite index already covers the query.
• Parallelising multiplies pressure downstream. Ten concurrent workers make ten times the queries; the bottleneck moves to the database and gets harder to see.
• Micro-optimisations can change semantics. Replacing a Decimal with a float, or a stable sort with an unstable one, is a correctness change wearing a performance costume.

How not to over-engineer the fix

This is the most over-corrected category in code review, because performance work feels productive and is easy to justify.

The measurement rule. If the defect is a shape (a query in a loop, an unbounded read, a quadratic scan), you can call it out from the diff — the complexity class is visible and the fix is local. If the concern is a constant factor ("this map lookup could be an array", "this allocates"), it needs a measurement before it justifies a comment, and probably does not belong in this review at all.

The proportionate fixes, cheapest first:

1. Move the I/O out of the loop. One query with an IN clause, one batched fetch, one prepared statement reused.
2. Add the bound. A LIMIT, a page size, a max upload size, a cache with a maximum entry count and a TTL.
3. Stream instead of buffering. Iterate lines, not readlines(). Return a cursor, not a list.
4. Aggregate locally, merge once. The standard fix for cross-process contention: each worker keeps its own counters and merges at the end, removing both the lock and the IPC.
5. Only then consider a cache, an index, denormalisation, or parallelism.

Push back on: introducing Redis to fix a missing JOIN; async rewrites of code that is not I/O bound; object pools and hand-rolled buffer reuse in a request path that makes a network call anyway; a queue and a worker fleet for something that takes 40ms; and any optimisation whose justification is "this could be slow later" with no n attached.

Comment templates that an agent can act on

• "inventoryRepository.getStock(...) runs inside the item loop — one query per line item. Fetch all product ids in a single query before the loop and index by id."
• "f.readlines() loads the whole file into memory; these logs are multi-gigabyte. Iterate the file object line by line instead."
• "shared_stats is a Manager.dict mutated once per line, so every increment is an IPC round trip. Accumulate in a local dict per chunk and merge the returned dicts."
• "This cache has no eviction or TTL and is keyed by user id — it grows without bound. Use an LRU with a maximum size."
• "results.contains(item) inside the loop makes this O(n²) over a list that reaches ~50k. Build a HashSet before the loop."