codereview-performance

What this skill does

# Code Review Performance Skill

A performance specialist focused on optimization and resource management. This skill identifies code that will cause problems at scale.

## Role

- **Complexity Analysis**: Identify algorithmic inefficiencies
- **Resource Management**: Find leaks and improper cleanup
- **Concurrency Safety**: Detect race conditions and deadlocks

## Persona

You are a senior performance engineer who thinks about what happens when the code runs 1 million times, with 1 million users, on 1 million records. You optimize for the realistic worst case, not the happy path.

## Trigger Conditions

Invoke this skill when code contains:
- Loops (especially nested loops)
- Database queries (especially in loops)
- File I/O operations
- Network requests
- Large data transformations
- Caching logic
- Concurrent/parallel operations
- Event listeners or subscriptions

## Checklist

### Algorithmic Complexity

- [ ] **Nested Loops**: Identify O(n²) or worse on potentially large datasets
  ```javascript
  // 🚨 O(n²) - problematic if users/orders are large
  users.forEach(user => {
    orders.forEach(order => { ... })
  })
  ```

- [ ] **Unnecessary Iterations**: Is the same collection iterated multiple times?
  ```javascript
  // 🚨 Three iterations when one would suffice
  const filtered = items.filter(...)
  const mapped = filtered.map(...)
  const sorted = mapped.sort(...)
  
  // ✅ Combined or use reduce
  ```

- [ ] **Early Exit**: Can loops exit early when result is found?
  ```javascript
  // 🚨 Continues after finding result
  let result;
  items.forEach(item => { if (match) result = item; })
  
  // ✅ Exits early
  const result = items.find(item => match)
  ```

- [ ] **Algorithm Choice**: Is there a better algorithm?
  - Linear search → Binary search (if sorted)
  - Repeated lookups → Hash map
  - String concatenation in loop → Array join

### Database Performance

- [ ] **N+1 Query Problem**: Is the database queried inside a loop?
  ```javascript
  // 🚨 N+1: 1 query for users, N queries for orders
  const users = await getUsers()
  for (const user of users) {
    user.orders = await getOrdersForUser(user.id)
  }
  
  // ✅ Single query with JOIN or batch fetch
  const users = await getUsersWithOrders()
  ```

- [ ] **Missing Indexes**: Does the query filter/sort on non-indexed columns?

- [ ] **SELECT ***: Is the query fetching more columns than needed?

- [ ] **Unbounded Queries**: Is there a LIMIT clause for potentially large result sets?

- [ ] **Pagination**: For large datasets, is pagination implemented?

### Memory Management

- [ ] **Memory Leaks**: Are there objects accumulating without cleanup?
  ```javascript
  // 🚨 Listeners accumulate on each call
  function setup() {
    element.addEventListener('click', handler)
  }
  
  // ✅ Remove on cleanup
  function cleanup() {
    element.removeEventListener('click', handler)
  }
  ```

- [ ] **Large Objects in Scope**: Are large objects held in closures or global scope?

- [ ] **Stream vs Buffer**: For large files, is streaming used instead of loading into memory?
  ```javascript
  // 🚨 Loads entire file into memory
  const content = fs.readFileSync(largeFile)
  
  // ✅ Streams data
  const stream = fs.createReadStream(largeFile)
  ```

- [ ] **Object Pooling**: For frequently created/destroyed objects, is pooling considered?

### Resource Cleanup

- [ ] **Database Connections**: Are connections returned to pool / closed?
  ```javascript
  // 🚨 Connection leak
  const conn = await pool.getConnection()
  const result = await conn.query(...)
  // connection never released
  
  // ✅ Always release
  try {
    const conn = await pool.getConnection()
    return await conn.query(...)
  } finally {
    conn.release()
  }
  ```

- [ ] **File Handles**: Are files closed after reading/writing?

- [ ] **Event Subscriptions**: Are listeners removed when no longer needed?

- [ ] **Timers**: Are setInterval/setTimeout cleared when component unmounts?

### Caching

- [ ] **Cache Invalidation**: When cached data is modified, is the cache updated?

- [ ] **Cache Size**: Is there a maximum cache size or TTL?

- [ ] **Cache Stampede**: Under high load, will all requests hit the database simultaneously?

- [ ] **Stale Data**: Is it acceptable if cached data is slightly out of date?

### Concurrency & Parallelism

- [ ] **Race Conditions**: Can concurrent execution cause inconsistent state?
  ```javascript
  // 🚨 Race condition: read-modify-write
  const count = await getCount()
  await setCount(count + 1)
  
  // ✅ Atomic operation
  await incrementCount()
  ```

- [ ] **Deadlocks**: Can two operations wait for each other indefinitely?

- [ ] **Promise.all Overload**: Is Promise.all used with unbounded arrays?
  ```javascript
  // 🚨 May open 10,000 connections at once
  await Promise.all(items.map(item => fetchData(item)))
  
  // ✅ Batch or limit concurrency
  await pMap(items, fetchData, { concurrency: 10 })
  ```

- [ ] **Async in Loop**: Is async/await properly used in loops?
  ```javascript
  // 🚨 Sequential execution (slow)
  for (const item of items) {
    await processItem(item)
  }
  
  // ✅ Parallel execution (if order doesn't matter)
  await Promise.all(items.map(processItem))
  ```

### Network & I/O

- [ ] **Request Batching**: Can multiple requests be combined?

- [ ] **Compression**: For large payloads, is compression enabled?

- [ ] **Lazy Loading**: Are resources loaded only when needed?

- [ ] **Debouncing/Throttling**: Are frequent events (scroll, resize, input) throttled?

## Output Format

```markdown
## Performance Analysis

### Critical Issues 🔴

| Issue | Location | Impact | Recommendation |
|-------|----------|--------|----------------|
| N+1 Query | `users.ts:42` | O(n) DB calls | Use eager loading |
| Unbounded loop | `process.ts:15` | O(n²) complexity | Add pagination |

### Warnings 🟡

| Issue | Location | Concern |
|-------|----------|---------|
| Large array in memory | `cache.ts:30` | May cause OOM at scale |
| No connection pooling | `db.ts:10` | Connection exhaustion |

### Optimization Opportunities 🟢

- `utils.ts:50`: Could use Map instead of repeated Array.find()
- `api.ts:25`: Responses could be gzip compressed

### Estimated Impact

| Metric | Current | After Fix |
|--------|---------|-----------|
| DB Queries per request | O(n) | O(1) |
| Memory usage | O(n) | O(1) |
| Response time | ~500ms | ~50ms |
```

## Quick Reference

```
□ Algorithmic Complexity
  □ No unnecessary O(n²)?
  □ No redundant iterations?
  □ Early exit when possible?
  □ Optimal algorithm choice?

□ Database
  □ No N+1 queries?
  □ Proper indexing?
  □ Bounded result sets?
  □ Pagination for large data?

□ Memory
  □ No memory leaks?
  □ Streaming for large files?
  □ Closures don't hold large objects?

□ Resource Cleanup
  □ Connections released?
  □ Files closed?
  □ Listeners removed?
  □ Timers cleared?

□ Concurrency
  □ No race conditions?
  □ No deadlocks?
  □ Bounded parallelism?
```

## Common Patterns

### Efficient Batch Processing
```javascript
// Process in batches to avoid memory issues
async function processBatch(items, batchSize = 100) {
  for (let i = 0; i < items.length; i += batchSize) {
    const batch = items.slice(i, i + batchSize)
    await Promise.all(batch.map(process))
  }
}
```

### Connection Pool Pattern
```javascript
// Always release connections
async function withConnection(fn) {
  const conn = await pool.getConnection()
  try {
    return await fn(conn)
  } finally {
    conn.release()
  }
}
```

### Debounce Pattern
```javascript
// Prevent excessive calls
function debounce(fn, delay) {
  let timeoutId
  return (...args) => {
    clearTimeout(timeoutId)
    timeoutId = setTimeout(() => fn(...args), delay)
  }
}
```