Repository navigation
perf(relay): cache process-table descendant indexes - #17646
Conversation
5c98709 to
55b1028
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe process-table reader now shares cached lenient and strict parses for each capture. Process indexes cache PID and parent-child lookups by snapshot identity and preserve the first row for duplicate PIDs. Foreground-process lookup reuses the index for root and descendant traversal, with centralized candidate scoring. Tests cover capture behavior, index memoization, duplicate rows, and foreground resolution. Merge Risk: 🔵 Low · up to The cache reduces repeated process-table work for multiple terminal panes but adds a measured single-pane cost of about 5–33 microseconds per 500 ms window before breaking even at two to three panes. This bounded performance tradeoff is mergeable with owner awareness; no merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the change, motivation, implementation, testing, performance impact, and scope. However, the required Linked Issue section is marked N/A even though the template requires an issue link.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR memoizes process-table indexes by snapshot-array identity while preserving the relay’s previous first-match PID semantics.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/shared/process-table-snapshot.ts | Adds weak identity-based index memoization and changes duplicate PID indexing to preserve first-row lookup semantics. |
| src/relay/pty-shell-utils.ts | Reuses the shared process-table index for root resolution and descendant traversal instead of rebuilding a map per pane. |
| src/shared/process-table-snapshot.test.ts | Covers memoization identity, snapshot separation, first-wins duplicate handling, index shape, and stats isolation. |
| src/relay/pty-shell-utils.test.ts | Verifies relay foreground-process resolution when malformed process output repeats the root PID. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[TTL-cached process-table snapshot] --> B[getProcessTableIndex]
B --> C{Index cached for array identity?}
C -->|No| D[Build byPid and childrenByPpid]
D --> E[Store in WeakMap]
C -->|Yes| F[Reuse cached index]
E --> G[Resolve PTY root and descendants]
F --> G
Reviews (4): Last reviewed commit: "fix(relay): keep the process-table index..." | Re-trigger Greptile
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
getProcessTableIndexmemo — aWeakMap<readonly ProcessTableRow[], ProcessTableIndex>insrc/shared/process-table-snapshot.tscaches one index per snapshot-array identity;getProcessTableSnapshot()returnscached.valuedirectly, so identity is stable within the 500ms TTL and the memo actually hits.- Relay path reuses the index —
collectDescendantsnow walksindex.childrenByPpidand root resolution goes throughindex.byPid.get(pid), eliminating the per-pane parent/child rebuild insrc/relay/pty-shell-utils.ts. - Two tests files — identity reuse/isolation, last-wins tie-break, and a stats-hazard guard (
getProcessTableIndexis stats-free so a cache hit can't satisfy a measuredindexBuilds) inprocess-table-snapshot.test.ts; the duplicate-root-pid case inpty-shell-utils.test.ts.
The build order in collectDescendants is byte-identical to the old childrenByParent (same iterate+push, same DFS), so the only behavioral change is the first-wins → last-wins root resolution on malformed/duplicate-pid captures — deliberate, documented, and pinned by tests. The WeakMap value's back-reference to rows (via index.rows) is inert under ephemeron semantics, and the memo is shape-agnostic with respect to the in-flight #17763.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — since the prior review, the author narrowed the index to the two maps the relay actually reads and restored first-wins pid resolution.
- Restored first-wins pid tie-break —
buildProcessTableIndexnow skips a duplicate pid (if (!byPid.has(row.pid))), reverting the relay root lookup from last-wins back to the originalrows.find()first-wins semantics and applying the same rule toresolveAgentForegroundProcessesFromIndex. - Dropped the unread
byPgid/byTpgidmaps —ProcessTableIndexand its builder no longer materialize the two maps no resolver queries, so a one-pane relay pays for two maps per capture instead of four. - Tests updated and extended — duplicate-pid assertions now pin first-wins for both the memo and the shared builder, and a new test pins the two-map index shape.
I verified the removed maps have no remaining readers anywhere in the repo, the batch resolver touches only byPid/childrenByPpid, and the touched suites pass (process-table-snapshot.test.ts, pty-shell-utils.test.ts, agent-foreground-process-batch.test.ts — 59 tests) with clean lint on the changed files.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Two defects in the memoized index this PR introduced. - Restore the first-wins duplicate-pid tie-break the relay had as `rows.find()`. A process whose argv contains a newline makes `ps` print a continuation line that the lenient parser can accept as a spurious row duplicating a real pid; that row always FOLLOWS the real one, so last-wins let it capture the pane's foreground. The rule now lives in `buildProcessTableIndex`, so the batched evidence resolver's `byPid.get(rootPid)` root lookup gets the same semantics the subsystem had before indexing. - Build only the two indexes a resolver reads. `byPgid`/`byTpgid` have no readers repo-wide, and delegating to a four-map build made a one-pane relay pay more per 500ms capture than the single `childrenByParent` map it replaced -- a regression in the majority topology, in a PR whose point is relay CPU. Matches the same deletion in #17763 line for line so whichever merges second resolves trivially.
fe50c21 to
df1988b
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior review, the branch was rebased onto main (which now includes #17525 and #17763) and squashed from three commits into two (de5b084e25 + df1988b6a2). The author's own diff is unchanged in substance — the memo, the first-wins tie-break, and the two-map index were already reviewed and approved — and the only textual delta is the main-side #17763 surface the rebase picked up.
- Rebased onto #17763 —
getForegroundProcessNameFromProcessTablenow usesmain's singleProcessTableCapturereader and sharedscoreForegroundCandidateRowin place of the former dual-reader layout and localcandidateScore; these aremain's changes, not new behavior introduced here. - First-wins tie-break re-applied —
buildProcessTableIndexretainsif (!byPid.has(row.pid))(first-wins, matchingrows.find()), and thebyPgid/byTpgiddeletion thedf1988b6a2commit message references is already onmainvia #17763, so it no longer appears in this PR's diff. - Tests re-verified — the duplicate-root-pid test now asserts first-wins (
'codex'), and the new tests pin the two-map index shape and the stats-free memo; each is written to fail under the unwound behavior.
I verified the strict parser used by the batch evidence path throws on malformed rows, so the first-wins change is unreachable there (no behavior change for resolveAgentForegroundProcessesFromIndex), and the memo's identity key is stable because ProcessTableCapture.lenient() memoizes the parse per capture.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
* perf(relay): cache process-table descendant indexes * fix(relay): keep the process-table index first-wins and narrow Two defects in the memoized index this PR introduced. - Restore the first-wins duplicate-pid tie-break the relay had as `rows.find()`. A process whose argv contains a newline makes `ps` print a continuation line that the lenient parser can accept as a spurious row duplicating a real pid; that row always FOLLOWS the real one, so last-wins let it capture the pane's foreground. The rule now lives in `buildProcessTableIndex`, so the batched evidence resolver's `byPid.get(rootPid)` root lookup gets the same semantics the subsystem had before indexing. - Build only the two indexes a resolver reads. `byPgid`/`byTpgid` have no readers repo-wide, and delegating to a four-map build made a one-pane relay pay more per 500ms capture than the single `childrenByParent` map it replaced -- a regression in the majority topology, in a PR whose point is relay CPU. Matches the same deletion in stablyai#17763 line for line so whichever merges second resolves trivially.
ELI5
psoutput is cached for 500ms, so a burst of terminal panes all inspect the same rows array. Each pane still walked every row to rebuild its own parent→child map before looking for the agent process. This memoizes one index per snapshot, keyed by the array itself, so the first pane in a window pays for the walk and the rest get it free. TheWeakMapkey means the index is released along with the snapshot.What Changed
Rebased onto current
mainand re-expressed on top of #17525, which has since merged.src/shared/process-table-snapshot.ts— addgetProcessTableIndex(rows): aWeakMap<readonly ProcessTableRow[], ProcessTableIndex>memo sitting directly on top of feat(ssh): batch process evidence in PTY inventory #17525'sbuildProcessTableIndex. No new index type and no second builder;main'sProcessTableIndexremains the only one.src/relay/pty-shell-utils.ts—collectDescendantstakes the shared index instead of rebuilding a privatechildrenByParentmap per call, andgetForegroundProcessNameFromProcessTableresolves the PTY root throughindex.byPidinstead ofrows.find().Why
getForegroundProcessNameruns per pane on the 750ms/2000ms inspection cadence and reads the TTL-cached snapshot, so N panes in one window each did an O(rows) map build over an identical array. Now that is one build per capture. #17525 batched the inventory path but left this per-pane relay path rebuilding.Two Deliberate Decisions
Duplicate-pid tie-break — kept first-wins, and moved the rule into the shared builder.
rows.find()was first-wins;buildProcessTableIndexwas last-wins (byPid.set), so routing the relay through the shared index silently flipped root resolution on a malformed capture. First-wins is restored, and the rule now lives inbuildProcessTableIndexsoresolveAgentForegroundProcessesFromIndex'sbyPid.get(rootPid)root lookup gets the same semantics the subsystem had before indexing.The tie-break is reachable and not symmetric. A process's argv may legally contain a newline, and
ps -axo ... command=prints it raw, so the lenient parser can accept a continuation line as a spurious row duplicating a real pid — and that spurious row always follows the real one it was split from. Last-wins therefore hands the pane's foreground identity to a row assembled from process-controlled argv bytes. First-wins keeps the real root. Pinned byresolves a duplicated root pid to the FIRST capture row(relay:'bash'→'codex') andresolves a duplicated pid to the FIRST row, as \rows.find()` did(shared, asserted against bothgetProcessTableIndexandbuildProcessTableIndex`); both confirmed red against the previous commit.The memo builds only the two indexes a resolver reads. Delegating to a four-map
buildProcessTableIndex(byPid,childrenByPpid,byPgid,byTpgid) made this PR slower in the topology it is meant to help: the relay path reads onlybyPidandchildrenByPpid, and the code it replaced built exactly one map. A single-pane relay would have gone from ~3 map/array ops per row per 500ms window to ~10.byPgid/byTpgidhave no readers anywhere in the repo, so they are dropped — the same deletion #17763 makes, written line for line to match it. Pinned bymaterializes only the maps the descendant walk reads.Per 500ms TTL window, over
Nrows andPpanes polling in that window:find+ walk)Those are op counts, not timings. Measured wall clock (Node 24.18, macOS arm64, one process per variant, real bundled
getForegroundProcessNameFromProcessTablefrom each revision; 20 000 warmed windows per config; a fresh rows-array copy per window so theWeakMapkey turns over exactly as it does in production, with the copy outside the timed region; root pid placed mid-table somain'srows.findpays its average scan). Median µs per 500 ms window:400-row table
main2000-row table
mainStated plainly: at 1 pane this is a real regression, not a wash. It costs ~1.5–2.4x at 400 rows (
mainranged 4.9–7.8 µs across repeated runs, this branch a stable 11.9 µs) and ~2.9x at 2000 rows. In absolute terms that is +5 µs per 500 ms window at 400 rows and +33 µs at 2000 rows — single-digit to low-tens of microseconds against a 500 000 µs window, so it is a regression worth stating plainly but not one a user can perceive. Break-even is between 2 and 3 panes. Past that the win grows withP: 1.6x at 8 panes and 2.2x at 20 panes on a 400-row table, and 4.2x at 20 panes on a 2000-row table.The wall-clock win is smaller than the op-count table above suggests (~12x fewer map/array ops at 20 panes) because the memo removes only the index build. The descendant walk, the candidate sort and
recognizeAgentProcessFromCommandLinestill run once per pane and set the floor: at 20 panes over 400 rows this branch spends ~12 µs building the one index and ~1.7 µs per pane after it. Any claim of a ~10x end-to-end speedup would be reading the op-count ratio as if it were wall clock.The four-map version was a worse 1-pane regression than "~3x" — 24.9 µs vs 5.0 µs (~5x) at 400 rows, 117.8 µs vs 17.4 µs (~6.8x) at 2000 rows — which is what motivated narrowing the build to the two maps a resolver reads.
The memo is deliberately stats-free.
buildProcessTableIndex(rows, stats)mutates the caller's counter bag and stores it on the returned index, andlookupProcessTableIndexincrements it afterwards. Memoizing such an index would hand one caller's bag to an unrelated later caller and let a cache hit satisfy anindexBuildsassertion without building anything.getProcessTableIndextherefore takes nostatsand never memoizes a measured build; the perf gate inagent-foreground-process-batch.test.tsstill callsbuildProcessTableIndex(rows, stats)directly and still measures a real build. Pinned bykeeps the memo out of measured builds so a cache hit cannot satisfy a perf gate.Notes On The Rebase
src/shared/process-table-snapshot.tsauto-merged with no conflict markers into a file declaringexport type ProcessTableIndextwice —git statusreported it clean and every test still passed; onlytsccatches it (TS2300). Resolved by takingmain's file wholesale and re-adding only the memo.src/relay/pty-shell-utils.tsandsrc/shared/process-table-snapshot.test.tswere resolved tomain's feat(ssh): batch process evidence in PTY inventory #17525 structure, then the memo was reapplied on top.[...evidenceRows]spread this PR originally needed to remove no longer exists: feat(ssh): batch process evidence in PTY inventory #17525 rewrotelistProcessesto publish titles fromevidenceResults[entryIndex], so nothing copies the rows array and the identity key is intact. For the same reason the "hoist one index across both the batch resolver and the title path" idea is already satisfied onmain— a listing walks the table once. No change needed there.Interaction With #17763
#17763 (
brennanb2025/r1-build-review-fixes) edits the same files: it collapses the twopsreaders into oneProcessTableCapturewith lazylenient()/strict()parsers and dropsbyPgid/byTpgid. There is no merge-ordering constraint — thebyPgid/byTpgiddeletion is duplicated here rather than deferred, so this branch stands on its own at the cost measured above (a small 1-pane regression, break-even by ~3 panes, a growing win beyond) instead of needing #17763 to land first to narrow the build.ProcessTableCapture.lenient()memoizes its parse per capture, so snapshot array identity — the thing this cache keys on — survives.lookupProcessTableIndexso fix(relay): close review findings from #17525 #17763's insertion ofscoreForegroundCandidateRowlands on unchanged context.add1f1c8a48:process-table-snapshot.tsnow has exactly one 1-line conflict (both sides rewrite thereturn { ... }line; fix(relay): close review findings from #17525 #17763's side is correct as-is). The other three conflicts — two import lists and thegetForegroundProcessNameFromProcessTablebody — are unchanged from before this commit and are inherent to both PRs rewriting that function.Linked Issue
N/A — proactive performance audit.
Visual Proof
N/A— relay/shared process bookkeeping only; no UI or interaction change.Testing
pnpm tc:node— clean (run first; the only check that catches the silent duplicate-type auto-merge).pnpm test src/shared/process-table-snapshot.test.ts src/relay/pty-shell-utils.test.ts src/main/providers/— 858 passed, 5 skipped.pnpm exec oxlint/pnpm exec oxfmt --checkon all four changed files — clean.Review
Ready for review. The earlier "#17525 may supersede this; do not merge both implementations" note is obsolete — #17525 has merged as
9477b5fcbb9, this branch now builds on it rather than duplicating it, and the overlap it warned about is gone.No RPC, wire, persistence, mobile schema, provider, or path contract changes are included.
Agent skill upstream boundary
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (CI will cover; focused local checks listed above)