Task 118: Compact Cell Representation (scrollback memory ~3-4x) - #389
Merged
Conversation
…ne bench Task 118 phase-one groundwork. Adds a durable public measurement API (Buffer::heap_bytes -> BufferHeapBreakdown) reporting capacity-based heap bytes held by Buffer.rows and Buffer.row_cache, with Arc<Url> payloads deduplicated by pointer identity. Adds benches/buffer_memory_bench.rs building three synthetic scrollback corpora (shell_session, source_logs, high_entropy_colored) and printing a bytes-per-scrollback-line report. This is the 'before' baseline captured on current main ahead of the compact-cell-representation change. Baseline (4000 scrollback lines, 80x24, capacity-based): shell_session : 6237 B/line source_logs : 5665 B/line high_entropy_colored : 10207 B/line
Task 118 phase-one. Adds freminal-buffer/src/compact_row.rs: a pure, format-run-sharing compact representation of a scrollback Row that stores formatting and wide-glyph bookkeeping run-length-encoded instead of a full 40-byte FormatTag per cell. - CompactRow::from_row (returns None for image rows; image rows opt out) - CompactRow::to_row (exact lossless rebuild) - is_compactable gate; heap_bytes accounting for measurement - Cell::from_parts (pub(crate)) reconstruction primitive preserving arbitrary wide-glyph flag combinations (orphan continuations, etc.) - Compile-time size assertions: Cell == 72 bytes, FormatTag == 40 bytes (previously unasserted; now measured and locked) Pure data transform: no Buffer integration (that is 118.3), no codec (that is Task 119). 12 exhaustive round-trip tests; 511 lib tests green.
…sions Captures the durable design decisions made at 118.3 activation against the real code: - Design B (Row-internal storage enum) over Design A (Buffer.rows as Vec<StoredRow>), chosen on blast-radius: A touches ~228 rows[...] sites vs B confining the change to row.rs. Zero hot-path cost either way, so correctness-risk decides. - Decompact-all-on-resize: recon disproved the read-only-scrollback assumption (set_size/resize_height dirty scrollback in place; image clears scan all rows but only hit opt-out image rows). Resize is rare and already O(all rows), so bracketing it with decompact/recompact adds no hot-path cost and keeps the fragile resize logic untouched. - Adds cleanup entry 118.7 for the resize_height 0..old_height dirty-pass smell surfaced during recon.
Store scrollback rows in a compact, format-run-sharing representation and
evict their redundant caches, cutting resident scrollback memory ~3-4x;
raise the default scrollback from 4000 to 10000 at net-neutral memory.
118.3/118.4 (buffer core):
- Row gains an internal RowStorage { Live(Vec<Cell>), Compact } enum;
all public accessors stay identical (Buffer's rows[...] sites untouched).
Compact rows materialize on read via a memoized OnceCell and decompact
in place on any mutation (ensure_live).
- CompactRow wired in: is_compactable gates image rows out; lossless.
- Compacted scrollback rows evict their RowCacheEntry and release their
decompaction memo; re-reading rebuilds transparently.
- Buffer::compact_idle_scrollback(budget): deferred-compaction entry
point (driven by the PTY idle tick, next commit). No hot path compacts.
- Reflow offset accounting O(n^2) -> O(n); no blanket decompact-on-resize.
118.5 (default scrollback 4000 -> 10000, chosen with data):
- Measured settled cost ~1.0-1.7 KB/line after compaction, so 10000 lines
~= 17 MB ~= the old 4000-line default's ~16.6 MB: 2.5x history at
net-neutral steady-state memory.
- ScrollbackConfig::default + buffer compiled-in fallback kept in sync;
config_example.toml + all default-assertion tests updated.
cargo test --all green.
…thread Compaction is a background memory optimization, never synchronous on a hot path: the PTY consumer thread compacts scrollback only when idle. - Add a real idle-tick arm to the PTY select! loop (crossbeam after(100ms)), firing only when neither PTY data nor GUI input has arrived. The arm compacts a bounded budget (512 rows) via compact_idle_scrollback, re-arms while work remains, and disarms (never()) once caught up so a quiescent pane is not woken forever. Respects the lock-free architecture: the PTY thread still owns TerminalEmulator exclusively. - The idle arm never calls post_event (compaction is snapshot-invisible), so it never triggers a spurious GUI wake. - After the backlog drains, release freed heap to the OS via malloc_trim (glibc only; no-op elsewhere). glibc retains freed small-allocation pages in its arenas, so without this RSS stayed ~800 MB after a 100k-line cat even though the live set was ~145 MB; with it, RSS falls to ~384 MB. - TODO noted: make compaction pacing dynamic based on outstanding backlog. cargo test --all green; cargo xtask check-windows green.
The original memory harness gave false confidence: it built buffers without ever rendering during the fill, so scrollback rows never accrued the RowCacheEntry they retain in the live app, and it measured only internal accounting (which stayed correct while process RSS did not). Both gaps let real leaks pass unnoticed. - Fill now flattens the visible window periodically during the fill (FLATTEN_EVERY_LINES; chunked build_snapshot for the real-parser bench), so scrollback rows carry stale caches exactly as in production. This reproduces the ~180 MB stale-cache condition a 100k scrollback showed. - Report three labelled states: fresh (pre-idle, transient), settled (post-idle-compaction, the real win), settled+search (Ctrl-F). - Add real process-RSS reporting (trimmed /proc/self/statm, Linux) as a per-corpus delta so an accounting-vs-RSS divergence (allocator retention) is visible rather than hidden. - Add real-world colored corpora fed through the real ANSI parser (build_output, ls_color) in the emulator-crate bench. cargo bench --no-run --all green.
…atus Update PLAN_VERSION_120.md for the Task 118 work landed in this series: - Durable decisions: Row-internal storage (Design B), decompact-on-resize, and the deferred/idle/budgeted compaction model (never on a hot path). - 118.5 marked done (default 4000 -> 10000, data-backed). - 118.7 marked resolved (resize_height dirty-range fix + regression test), folded into the 118.5 pass. - 118.8 marked obsolete (compact_newly_scrolled_rows removed by 118.9). - 118.9 subtask added (deferred idle-driven compaction + PTY idle tick). - 118.10 added as an enriched stub: windowed/lazy reflow, so a 100k-line scrollback never triggers a long blocking full reflow on resize.
📝 WalkthroughWalkthroughThe change adds compact scrollback row storage with lazy decompaction, deferred PTY-idle compaction, heap diagnostics, memory benchmarks, corrected resize/cache behavior, and a default scrollback increase from 4000 to 10000. Release metadata moves to ChangesScrollback compaction
Sequence Diagram(s)sequenceDiagram
participant Terminal as Terminal emulator
participant Buffer
participant Row
participant PTY as PTY idle loop
Terminal->>Buffer: append terminal output
Buffer->>Row: retain live scrollback rows
PTY->>Buffer: compact_idle_scrollback(budget)
Buffer->>Row: compact eligible scrollback
Terminal->>Buffer: flatten scrollback
Buffer->>Row: lazily decompact and rebuild cache
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
freminal-buffer/src/buffer/resize_and_alt.rs (1)
206-266: 📐 Maintainability & Code Quality | 🔵 TrivialConfirm the
SavedPrimaryStatescrollback-limit gap is tracked.The comment already discloses that
resize_saved_primary's tempBufferuses a hardcoded10_000instead of the user's real configured limit, so an alt-screen resize enforces the wrong scrollback limit for any pane whose configured limit differs from the default. This is pre-existing (previously hardcoded at4000) and explicitly called out as out-of-scope here — worth confirming a follow-up issue exists to thread the real limit throughSavedPrimaryState, since it's otherwise easy to forget once this comment scrolls out of view.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@freminal-buffer/src/buffer/resize_and_alt.rs` around lines 206 - 266, Track a follow-up issue to carry the configured scrollback limit through SavedPrimaryState and use it in resize_saved_primary instead of the hardcoded 10_000 value. Keep this change scoped to recording the follow-up; do not alter the current resize behavior.freminal-buffer/src/compact_row.rs (1)
37-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCrate-wide compile-time size assert is brittle.
size_of::<Cell>() == 72/size_of::<FormatTag>() == 40fire as hardconstassertions at ordinarycargo buildtime, not gated to tests. Any future unrelated field addition toCell/FormatTag(or building for a different pointer width) breaks compilation crate-wide with a cryptic-ish message, instead of just failing a targeted test in CI.♻️ Proposed fix: move to a #[test]
-const _: () = assert!( - core::mem::size_of::<Cell>() == 72, - "Cell size changed — re-measure CompactRow's space savings" -); -const _: () = assert!( - core::mem::size_of::<FormatTag>() == 40, - "FormatTag size changed — re-measure CompactRow's space savings" -); +#[cfg(test)] +#[test] +fn cell_and_format_tag_sizes_match_documented_space_savings() { + assert_eq!(core::mem::size_of::<Cell>(), 72, + "Cell size changed — re-measure CompactRow's space savings"); + assert_eq!(core::mem::size_of::<FormatTag>(), 40, + "FormatTag size changed — re-measure CompactRow's space savings"); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@freminal-buffer/src/compact_row.rs` around lines 37 - 48, Move the compile-time size assertions for Cell and FormatTag out of module-level const evaluation and into a targeted #[test], preserving the exact expected sizes and failure messages there. Keep ordinary cargo builds unaffected while ensuring the space-accounting test fails when either measured size changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Line 74: Update the toml dependency declaration in Cargo.toml from the
unavailable 1.1.3 release to the published 1.1.2+spec-1.1.0 version so
dependency resolution succeeds.
In `@freminal-buffer/benches/buffer_memory_bench.rs`:
- Around line 46-88: Replace the raw FFI in process_rss_bytes and trim_allocator
with a safe RSS helper such as sysinfo, removing the custom
libc_sysconf_pagesize and malloc_trim bindings. Preserve the benchmark’s ability
to obtain process RSS where supported and retain the None/no-op behavior on
unsupported platforms without adding unsafe code.
In `@freminal-buffer/src/buffer/mod.rs`:
- Around line 236-292: TODO
In `@freminal-buffer/src/compact_row.rs`:
- Around line 20-22: Update the module-level documentation in compact_row.rs to
remove the stale claim that CompactRow is not integrated with Buffer or
scrollback storage and that integration is a separate task. Replace it with
wording that accurately reflects the existing RowStorage::Compact usage and
Buffer::compact_idle_scrollback integration.
In `@freminal-buffer/src/row.rs`:
- Around line 237-249: Update take_cells to handle the RowStorage::Compact case
with unreachable!(), matching the invariant enforcement in cells_vec_mut.
Preserve the existing Live-cell extraction without cloning, but fail loudly if
CompactRow::to_row no longer produces RowStorage::Live.
---
Nitpick comments:
In `@freminal-buffer/src/buffer/resize_and_alt.rs`:
- Around line 206-266: Track a follow-up issue to carry the configured
scrollback limit through SavedPrimaryState and use it in resize_saved_primary
instead of the hardcoded 10_000 value. Keep this change scoped to recording the
follow-up; do not alter the current resize behavior.
In `@freminal-buffer/src/compact_row.rs`:
- Around line 37-48: Move the compile-time size assertions for Cell and
FormatTag out of module-level const evaluation and into a targeted #[test],
preserving the exact expected sizes and failure messages there. Keep ordinary
cargo builds unaffected while ensuring the space-accounting test fails when
either measured size changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02177e86-6757-4689-9900-3c15625fc8f1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Cargo.tomlDocuments/PLAN_VERSION_120.mdconfig_example.tomlflake.nixfreminal-buffer/Cargo.tomlfreminal-buffer/benches/buffer_memory_bench.rsfreminal-buffer/src/buffer/flatten.rsfreminal-buffer/src/buffer/lifecycle.rsfreminal-buffer/src/buffer/mod.rsfreminal-buffer/src/buffer/resize_and_alt.rsfreminal-buffer/src/cell.rsfreminal-buffer/src/compact_row.rsfreminal-buffer/src/lib.rsfreminal-buffer/src/row.rsfreminal-common/src/config.rsfreminal-common/tests/config_tests.rsfreminal-terminal-emulator/benches/buffer_benches.rsfreminal-terminal-emulator/src/interface.rsfreminal-terminal-emulator/src/state/internal.rsfreminal-terminal-emulator/src/terminal_handler/mod.rsfreminal-terminal-emulator/tests/shadow_handler.rsfreminal/Cargo.tomlfreminal/src/gui/pty.rs
Comment on lines
+46
to
+88
| #[cfg(target_os = "linux")] | ||
| fn process_rss_bytes() -> Option<usize> { | ||
| let statm = std::fs::read_to_string("/proc/self/statm").ok()?; | ||
| let resident_pages: usize = statm.split_whitespace().nth(1)?.parse().ok()?; | ||
| // SAFETY: sysconf(_SC_PAGESIZE) is a pure query with no preconditions. | ||
| let page_size = unsafe { libc_sysconf_pagesize() }; | ||
| Some(resident_pages * page_size) | ||
| } | ||
|
|
||
| #[cfg(not(target_os = "linux"))] | ||
| fn process_rss_bytes() -> Option<usize> { | ||
| None | ||
| } | ||
|
|
||
| /// Page size via `sysconf(_SC_PAGESIZE)`. Declared inline to avoid a `libc` | ||
| /// dev-dependency just for one constant. | ||
| #[cfg(target_os = "linux")] | ||
| unsafe fn libc_sysconf_pagesize() -> usize { | ||
| unsafe extern "C" { | ||
| fn sysconf(name: core::ffi::c_int) -> core::ffi::c_long; | ||
| } | ||
| // _SC_PAGESIZE == 30 on Linux/glibc. | ||
| const SC_PAGESIZE: core::ffi::c_int = 30; | ||
| let v = unsafe { sysconf(SC_PAGESIZE) }; | ||
| if v > 0 { v as usize } else { 4096 } | ||
| } | ||
|
|
||
| /// Ask the allocator to return free pages to the OS (glibc `malloc_trim`), so | ||
| /// an RSS reading reflects live memory rather than allocator-retained free | ||
| /// pages. Mirrors the live app's post-idle-compaction trim. No-op off glibc. | ||
| fn trim_allocator() { | ||
| #[cfg(all(target_os = "linux", target_env = "gnu"))] | ||
| { | ||
| // SAFETY: `malloc_trim` only releases already-free heap; it cannot | ||
| // affect live allocations. | ||
| unsafe extern "C" { | ||
| fn malloc_trim(pad: usize) -> core::ffi::c_int; | ||
| } | ||
| unsafe { | ||
| let _ = malloc_trim(0); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What is the minimum supported Rust version for unsafe extern blocks (RFC 3484) outside edition 2024?
💡 Result:
The minimum supported Rust version for unsafe extern blocks is Rust 1.82.0 [1]. While RFC 3484 introduced the requirement to mark extern blocks as unsafe starting in the 2024 Edition [2][3][4], the capability to use unsafe extern blocks was stabilized for all editions in Rust 1.82.0 [2][1]. Prior to this version, extern blocks did not require (and could not use) the unsafe keyword [5][6]. As of Rust 1.82.0, you can opt into this syntax in any edition to take advantage of features like marking specific functions or statics as safe [1][7].
Citations:
- 1: https://nlopes.dev/rust-timeline/1.82/
- 2: https://doc.rust-lang.org/edition-guide/rust-2024/unsafe-extern.html
- 3: Tracking Issue for RFC 3484: Unsafe Extern Blocks rust-lang/rust#123743
- 4: https://rust-lang.github.io/rfcs/3484-unsafe-extern-blocks.html
- 5: https://doc.rust-lang.org/stable/reference/items/external-blocks.html
- 6: https://doc.rust-lang.org/nightly/reference/unsafe-keyword.html
- 7: Stabilize unsafe extern blocks (RFC 3484) rust-lang/rust#127921
🏁 Script executed:
set -euo pipefail
# Find the benchmark file and the workspace/toolchain settings.
git ls-files freminal-buffer/benches/buffer_memory_bench.rs Cargo.toml rust-toolchain.toml rust-toolchain
printf '\n--- Cargo.toml (relevant lines) ---\n'
sed -n '1,220p' Cargo.toml
printf '\n--- rust-toolchain.toml (if present) ---\n'
if [ -f rust-toolchain.toml ]; then sed -n '1,120p' rust-toolchain.toml; else echo "missing"; fi
printf '\n--- benchmark file (relevant lines) ---\n'
sed -n '1,220p' freminal-buffer/benches/buffer_memory_bench.rsRepository: fredsystems/freminal
Length of output: 12252
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- unsafe / extern usage in the affected crate tree ---'
rg -n --hidden --glob 'freminal-buffer/**/*.rs' '\bunsafe\b|extern "C"|unsafe extern' freminal-buffer || true
printf '\n%s\n' '--- sysinfo dependency usage ---'
rg -n --hidden --glob 'Cargo.toml' 'sysinfo' .
printf '\n%s\n' '--- benchmark file references to RSS / trim ---'
rg -n --hidden 'process_rss_bytes|trim_allocator|malloc_trim|sysconf|sysinfo' freminal-buffer/benches/buffer_memory_bench.rsRepository: fredsystems/freminal
Length of output: 2129
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- freminal-buffer/Cargo.toml ---'
sed -n '1,260p' freminal-buffer/Cargo.toml
printf '\n%s\n' '--- references to workspace deps in freminal-buffer/Cargo.toml ---'
rg -n 'workspace = true|sysinfo|libc' freminal-buffer/Cargo.toml
printf '\n%s\n' '--- Cargo.toml snippets for sysinfo/libc workspaces ---'
sed -n '60,80p' Cargo.tomlRepository: fredsystems/freminal
Length of output: 2448
Avoid raw unsafe FFI here freminal-buffer/benches/buffer_memory_bench.rs:47-85 The crate’s no-unsafe guideline applies here too; use a safe RSS helper (for example sysinfo) and avoid the malloc_trim/sysconf bindings unless there’s an explicit exception.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@freminal-buffer/benches/buffer_memory_bench.rs` around lines 46 - 88, Replace
the raw FFI in process_rss_bytes and trim_allocator with a safe RSS helper such
as sysinfo, removing the custom libc_sysconf_pagesize and malloc_trim bindings.
Preserve the benchmark’s ability to obtain process RSS where supported and
retain the None/no-op behavior on unsupported platforms without adding unsafe
code.
Source: Coding guidelines
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Fix the macOS-only `Test macos-latest` CI failure and address the actionable CodeRabbit review comments on PR #389. macOS CI SIGABRT: - `NotificationRouter::{show_system,show_system_osc99}` spawn a detached `notify-rust` thread that, on macOS, funnels into the Objective-C notification runtime. Invoked from a non-`.app`-bundled `cargo test` binary it throws an uncatchable foreign exception, which Rust's unwinder converts to `abort()` (SIGABRT). Because the thread is detached, the abort lands nondeterministically during test teardown after all 845 tests report ok, taking the whole harness down. Gate the real OS spawn behind `cfg!(test)` so unit tests never touch the ObjC path. The routing policy the tests assert on is fully evaluated before the spawn, so no coverage is lost. Pre-existing latent race that Task 118's timing changes made reproduce. Review remediations: - compact_row.rs: correct the stale module doc that claimed CompactRow is not wired into scrollback storage; it is, via RowStorage::Compact and Buffer::compact_idle_scrollback. - compact_row.rs: move the crate-level `const` Cell/FormatTag size assertions into a targeted `#[test]` so an unrelated field or pointer-width change fails one CI test instead of `cargo build` crate-wide. - row.rs: make `take_cells` fail loudly via `unreachable!()` on the structurally-impossible Compact case, matching `cells_vec_mut`, rather than silently returning an empty Vec. - buffer/mod.rs: add unit tests for the public `Buffer::heap_bytes` diagnostic (rows_bytes shrinks after compaction; a URL shared across cells is counted once). - PLAN_VERSION_120.md: record cleanup entry 118.11 tracking the pre-existing SavedPrimaryState hardcoded-scrollback-limit gap.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Documents/PLAN_VERSION_120.md`:
- Around line 15-18: Update the Task 118 description in
Documents/PLAN_VERSION_120.md to distinguish the compact representation’s
approximate 8–12× reduction from the measured settled RSS improvement of
approximately 3–4×. Label 8–12× as a representation-level reduction and state
the 3–4× settled resident-memory result separately, including the corresponding
repeated section noted by the review.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fa0f73b-6fe1-4d00-9e78-ae73c8b05836
📒 Files selected for processing (7)
Documents/MASTER_PLAN.mdDocuments/PLAN_VERSION_120.mdDocuments/PLAN_VERSION_131.mdfreminal-buffer/src/buffer/mod.rsfreminal-buffer/src/compact_row.rsfreminal-buffer/src/row.rsfreminal/src/gui/notifications.rs
💤 Files with no reviewable changes (1)
- Documents/PLAN_VERSION_131.md
🚧 Files skipped from review as they are similar to previous changes (3)
- freminal-buffer/src/buffer/mod.rs
- freminal-buffer/src/compact_row.rs
- freminal-buffer/src/row.rs
Comment on lines
+15
to
+18
| - **Task 118 — Compact Cell Representation** (done): a buffer-layer memory optimisation that | ||
| shrinks stored scrollback rows ~8–12× by sharing formatting across runs and dropping the | ||
| always-null image pointer, plus idle-driven compaction off the hot path. Merged on this | ||
| branch. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Distinguish compact-representation savings from settled RSS savings.
The “~8–12×” figure describes the flat compact representation, whereas the measured settled resident-memory improvement is approximately 3–4×. Label the former as a representation-level reduction and state the latter separately so the opening does not overstate the user-visible memory gain.
Also applies to: 393-398
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Documents/PLAN_VERSION_120.md` around lines 15 - 18, Update the Task 118
description in Documents/PLAN_VERSION_120.md to distinguish the compact
representation’s approximate 8–12× reduction from the measured settled RSS
improvement of approximately 3–4×. Label 8–12× as a representation-level
reduction and state the 3–4× settled resident-memory result separately,
including the corresponding repeated section noted by the review.
This was referenced Jul 20, 2026
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
1 participant
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.
Task 118 — Compact Cell Representation (v0.12.0)
Phase-one scrollback memory optimization: store scrollback rows in a compact,
format-run-sharing representation, compacted lazily in the background so the
cost never lands on a hot path. Reduces resident scrollback memory ~3–4× and
raises the default scrollback from 4000 to 10000 lines at net-neutral memory.
Live-app validated: catting a 100k-line file into a pane went from ~820 MB
→ ~384 MB resident after the terminal settles.
What changed
Rowinternal storage enum (Live(Vec<Cell>)/Compact): scrollbackrows store formatting run-length-encoded instead of a full 40-byte
FormatTagper cell. All publicRowaccessors are unchanged, soBuffer's~228
rows[...]call sites are untouched. Compact rows materialize on read(memoized) and decompact in place on any mutation. Image rows opt out.
row_cacheeviction: a compacted scrollback row drops its second(flatten-cache) and third (decompaction-memo) copies; re-reading (Ctrl-F
search) rebuilds transparently. This was the source of a ~180 MB stale-cache
leak on a 100k scrollback.
thread compacts scrollback only when idle (100 ms after the last activity),
512 rows per tick, disarming once caught up. Never synchronous on a hot path
(ingest / render / resize). Respects the lock-free architecture — no separate
thread touches the buffer.
malloc_trimafter the compaction backlog drains: glibc retainsfreed pages in its arenas, so RSS stayed high even after the live set shrank;
trimming once per settle returns the pages to the OS (Linux/glibc only,
no-op elsewhere).
decompact-on-resize.
(~1.0–1.7 KB/line settled ⇒ 10000 lines ≈ 17 MB ≈ old 4000-line default).
resize_heightinvalidating the wrong rows (top-of-scrollback instead of the bottom-anchored visible window) + regression test.
scrollback rows accrue the stale cache they do in production) and report real
process RSS deltas — both gaps that let earlier bugs pass unnoticed.
Memory results (bytes per scrollback line, settled = steady state)
Post-search (Ctrl-F) memory returns to the settled figure — eviction reclaims
the transient copies.
CPU (vs pre-118 baseline, 15% threshold)
Hot paths clean or improved: ingest −4–9%,
scrollback_render(offset 1000)−18.6%, alt-screen switch −48–62%, visible flatten ±0.
softwrap_heavyback to~0% (was +45% before deferral). Nothing over the 15% threshold. Parser/snapshot
paths within ±6%.
Follow-ups captured (not in this PR)
triggers a long blocking full reflow on resize — reflow the visible region
synchronously, defer the rest. Enriched stub in the plan; decompose later.
Verification
cargo test --all,cargo clippy --all-targets --all-features -- -D warnings,cargo fmt --all -- --check,cargo machete,cargo xtask check-windows— allgreen on the final tree.
Summary by CodeRabbit