Bound Buffer.resize to populated lines (don't materialize the full scrollback ring) - #556
Open
evertjr wants to merge 1 commit into
Open
Conversation
The col-grow, col-shrink, and DEBUG post-condition loops iterated up to lines.maxLength (the full scrollback ring). The lazy subscript would materialize a fresh BufferLine for every nil slot, allocating the entire ring on each resize. Use lines.count instead so we only touch populated lines, and gate the post-condition loop behind #if DEBUG.
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
Buffer.resize()iteratesfor i in 0..<lines.maxLengthin three places: a col-grow loop that adjusts each line's column count before reflow, a col-shrink loop that trims lines after reflow, and aDEBUG:-labelled post-condition that asserts every row matchesnewCols.The lines collection is a
CircularBufferLineListwhose subscript materializes aBufferLineon read when the slot isnil:So every resize reads every slot up to the scrollback cap, allocating a
BufferLinefor each nil entry. A terminal that has only pushed 200 lines but is configured withscrollback = 50_000materializes ~50,000 BufferLines on the first resize. Lines that never carried content end up holding a full-widthdataarray sized to current cols.Discovered in Maestri (12 terminals ×
scrollback = 50_000):heapshowed 600,591BufferLineinstances totaling 1.83 GB inBufferLine.dataafter any window/sidebar resize, even for alt-screen terminals (vim/agents) with minimal real scrollback in their normal buffer. After the patch the same workload sits at thousands of BufferLines instead of hundreds of thousands, and process RSS dropped from ~3 GB at workspace startup to ~1 GB.Most consumers won't notice because the default scrollback is small, but the cost is real for anyone increasing it or running many terminals: it's
alloc-per-nil-slot × resize-count × terminal-count.The
DEBUG:label on the post-condition is a leftover comment, not an#if DEBUG— the assertion ships in release andabort()s the process if it triggers, while also being the loop that materializes the most slots.Fix
lines.countinstead oflines.maxLength. Slots beyondcounthaven't been pushed and aren't logical buffer rows — reflow, rendering, selection extraction, image accounting, andgetBufferAsDataalready bound their reads bycount.#if DEBUGso the materialization side effect doesn't ship in release. The assertion still runs in development builds with the samecount-bound iteration.Behavior is identical for populated rows — they still get resized exactly as before. Resize is also incidentally faster on terminals with low utilization since the work now scales with content instead of cap.
Tests
Manually verified in Maestri across the regression matrix: resize during idle and during heavy
seqoutput, resize while invim/nano/less(alt screen), shrink + grow with wrapped content, brand-new-terminal resize (lines.count == 0, loops no-op), cross-floor resize. All identical to upstream behavior. Memory check:heap | grep BufferLineshows counts proportional to actual pushed content (a few hundred per terminal) instead of one full ring per terminal.