(perf): coalesce PTY outputBuffer to cut per-session memory overhead - #75
Merged
Conversation
Each PTY onData event pushes one string onto session.outputBuffer. Under heavy streaming this produces thousands of array entries, each with its own V8 ArraySlot + String-object GC root. Audit Q8 estimated worst-case ~400 KB of per-session slot overhead. Fix: extract appendToOutputBuffer() (output-buffer.js) which collapses the array into a single concatenated string once its length exceeds 64 entries. Whole-entry front-trim (shift) fires first as before; after coalescing a single-entry overflow is trimmed to the last MAX_BUFFER_SIZE bytes then advanced past the first newline, ensuring replay never starts mid-ANSI- escape-sequence or mid-UTF-8-codepoint. Reattach loop in main.js is unchanged (iterates outputBuffer verbatim). Tests: 9 unit tests covering size cap, array-length bound, tail-suffix ordering, coalesce byte-equivalence, and line-boundary trim.
The original step order (trim → coalesce → byte-slice) caused a catastrophic history drop: coalescing N entries into one large spine (up to ~193 KB) meant the next trim call's shift() removed the entire spine atomically, leaving only ~64 KB retained instead of ~256 KB. Fix: coalesce FIRST (step 1), then trim small entries (step 2), then byte-slice single oversized entry at a line boundary (step 3). Step 1 also coalesces when the buffer is over budget and the leading entry is itself ≥ max — this covers the post-byte-slice scenario where one new small chunk is pushed onto a ~max spine, preventing step 2's shift() from atomically evicting the spine on the very next push. Also: - Export MAX_BUFFER_SIZE from output-buffer.js; import it in main.js (single source of truth, eliminates silent divergence risk) - Add min-floor assertion to Test 3 (must retain ≥ MAX − one chunk); this test fails on the pre-fix code, passes on the fixed code - Import COALESCE_THRESHOLD and MAX_BUFFER_SIZE in the test instead of hardcoding; remove the stale per-test re-declaration of COALESCE_THRESHOLD - Document the no-newline fallback limitation at module level and update step-3 comment to accurately describe both reachable paths Reviewed in outputbuffer-review-75.md — BLOCKER + MAJOR + MINORs fixed.
The second coalesce trigger gated on outputBuffer[0].length >= max, but step 3's nl=0 line-boundary slice produces a spine of exactly max-1 bytes. A subsequent small chunk then tipped the buffer over budget while the spine slipped under the guard, letting step 2 shift() the whole max-1 spine in one call — the same atomic-history-drop the BLOCKER fix aimed to kill, just one byte below the threshold. Drop the first-entry size check so any over-budget multi-entry buffer coalesces, then step 3 byte-slices at a line boundary. Adds a regression test (MAX-1 spine + small chunk) that fails on the guarded code and passes here. Addresses WARNING from PR #75 re-review.
abasiri
pushed a commit
that referenced
this pull request
Aug 1, 2026
The push-to-talk handler (#22) intercepted every plain-Space keydown, calling preventDefault() and writing a raw space straight to the PTY. xterm's _keyDown runs the custom key handler BEFORE its composition helper, so during Korean/Japanese/Chinese composition this blocked the commit and reordered/dropped the in-progress syllable (e.g. "녕 " came out as " 녕"). Gate the direct-send path on !isImeComposing(e) (isComposing, or the Chromium keyCode 229 sentinel) so it only fires when no IME is composing — exactly when push-to-talk needs it. During composition the event now falls through to xterm's composition helper for correct commit behavior. Extracts the decision as a pure, exported predicate and adds unit tests. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Each PTY
onDataevent callssession.outputBuffer.push(data), appending one string per event. Under heavy streaming (ink-style TUI redraws, fast model output) this produces thousands of array entries per session. Each entry carries V8 ArraySlot metadata + a separate String-object GC root. Audit Q8 estimated worst-case ~400 KB of per-session slot overhead beyond the actual byte content — about 2× the useful data for typical sessions.Solution: coalesce-on-threshold
Extracted a pure
appendToOutputBuffer(state, data, max)helper (output-buffer.js) with three ordered steps:shift()leading chunks while over the 256 KB budget, but never empty the array here (length > 1guard).outputBuffer.length > 64, collapse the entire array into[outputBuffer.join('')]. This caps the per-session slot count to at most 65 entries (one coalesced spine + up to 64 new arrivals) while keeping the byte content identical.MAXbytes (newest content), then advance past the first\nso the retained string starts on a line boundary.The
for (const chunk of session.outputBuffer)reattach loop inmain.jsis unchanged — it iterates the same bytes in the same order regardless of how many entries the array has.Replay-safety guarantee
\nboundary ensures no mid-escape or mid-UTF-8-codepoint replay corruption.Estimated savings
Before: up to ~10 000 array slots per session (1 push per ~25-byte paint event over 256 KB → ~10 240 slots), each costing ≈40 bytes of V8 slot + String-object overhead → ~400 KB overhead per active session.
After: ≤65 slots regardless of event rate → overhead bounded to ~2.5 KB per session.
Practical sessions (lighter streaming): array stays under 64 entries so coalesce never fires, per-event cost is identical to baseline.
Tests
9 unit tests in
test/output-buffer.test.js(pure Node, no Electron):\nboundaryoutputBufferSizetracks actual byte count at every steptask checkresult in worktree: 256 pass, 30 fail — identical to the main branch baseline (the 30 failures are all pre-existing morphdom worktree false-negatives).