perf(ui): reduce highlight cache memory#404
Conversation
Greptile SummaryThis PR reduces the shared highlight cache cap from 150 to 40 entries to cut peak heap and RSS during navigation of large changesets, and adds two new harnesses to validate that memory stays bounded over time.
Confidence Score: 3/5The production code change is safe to merge; the daemon memory check script has a deadlock risk that should be fixed before relying on it in CI. The core production change is safe; the daemon harness has a defect that can cause it to silently deadlock rather than produce memory measurements. scripts/daemon-memory-check.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[useHighlightedDiff hook] --> B{Cache hit?\nSHARED_HIGHLIGHTED_DIFF_CACHE}
B -- Yes --> C[Return cached HighlightedDiffCode]
B -- No --> D{In-flight promise?\nSHARED_HIGHLIGHT_PROMISES}
D -- Yes --> E[Attach to existing promise]
D -- No --> F[loadHighlightedDiff async]
F --> G[commitHighlightResult]
G --> H[Add to cache]
H --> I[enforceCacheLimit\nmax 40 entries]
I --> J{size > 40?}
J -- Yes --> K[Evict oldest insertion-order entry]
K --> J
J -- No --> L[Done]
E --> C
C --> M[Render highlighted diff]
L --> M
|
| HUNK_MCP_HOST: "127.0.0.1", | ||
| HUNK_MCP_PORT: String(port), | ||
| HUNK_CONFIG_HOME: scratch, | ||
| HUNK_NO_UPDATE_CHECK: "1", | ||
| }, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const samples: MemorySample[] = []; | ||
|
|
||
| try { |
There was a problem hiding this comment.
reserveLoopbackPort releases the listener before the daemon process starts. The gap between listener.close() and the daemon's own bind() is a race window: another process on the same machine can acquire the same ephemeral port in between. When that happens the daemon fails to start, waitUntil("daemon health") times out after 10 s, and the script exits without a memory reading. On a busy CI host this race is non-trivial. A safer pattern is to pass a server socket by file descriptor (or use a Unix-domain socket path) so ownership is never released.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/daemon-memory-check.ts
Line: 505-516
Comment:
**Port TOCTOU — another process can steal the reserved port**
`reserveLoopbackPort` releases the listener before the daemon process starts. The gap between `listener.close()` and the daemon's own `bind()` is a race window: another process on the same machine can acquire the same ephemeral port in between. When that happens the daemon fails to start, `waitUntil("daemon health")` times out after 10 s, and the script exits without a memory reading. On a busy CI host this race is non-trivial. A safer pattern is to pass a server socket by file descriptor (or use a Unix-domain socket path) so ownership is never released.
How can I resolve this? If you propose a fix, please make it concise.| nextDirection = -1; | ||
| } else if (position <= 0) { | ||
| nextDirection = 1; |
There was a problem hiding this comment.
Once position reaches fileCount - 1, Math.min(options.fileCount - 1, position + 1) clamps back to the same index every iteration. All remaining navigations press ] at the boundary with no state change. Any post-warmup samples collected during these saturated navigations record flat heap readings (highlight already cached, no new work), which deflates the computed slope and makes the threshold check artificially easy to pass. The default options use navigations: 180 and fileCount: 90, so running with --mode forward and the defaults would silently saturate at navigation 90. A guard that logs a warning when options.navigations > options.fileCount in forward mode would surface this quickly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: benchmarks/navigation-memory.ts
Line: 193-195
Comment:
**Forward mode becomes a no-op past the last file**
Once `position` reaches `fileCount - 1`, `Math.min(options.fileCount - 1, position + 1)` clamps back to the same index every iteration. All remaining navigations press `]` at the boundary with no state change. Any post-warmup samples collected during these saturated navigations record flat heap readings (highlight already cached, no new work), which deflates the computed slope and makes the threshold check artificially easy to pass. The default options use `navigations: 180` and `fileCount: 90`, so running with `--mode forward` and the defaults would silently saturate at navigation 90. A guard that logs a warning when `options.navigations > options.fileCount` in forward mode would surface this quickly.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
Benchmarks
Navigation memory, forward through 220 unique files:
Large stream scroll benchmark, average of 3 runs:
The reduced cache keeps the nearby scroll/prefetch working set warm while avoiding retention of whole-review highlight data.
Validation
bun run typecheckbun run lintbun run benchmarks/large-stream.tsbun run benchmarks/navigation-memory.ts --mode forward --navigations 220 --warmup-navigations 150 --sample-every 20 --file-count 220 --lines-per-file 80 --max-heap-slope-kb 8192 --max-heap-growth-mb 1024 --max-rss-growth-mb 2048 --json-out tmp/navigation-memory-forward-220-cache40.jsonbun run bench:daemon-memory -- --json-out tmp/daemon-memory-default.jsonThis PR description was generated by Pi using GPT-5 Codex