diff --git a/freminal-buffer/benches/buffer_row_bench.rs b/freminal-buffer/benches/buffer_row_bench.rs index a53b8963..9bf2d4e9 100644 --- a/freminal-buffer/benches/buffer_row_bench.rs +++ b/freminal-buffer/benches/buffer_row_bench.rs @@ -362,6 +362,99 @@ fn bench_lf_heavy(c: &mut Criterion) { group.finish(); } +// --------------------------------------------------------------- +// Benchmark: steady-state LF at scrollback capacity — `merge_cache` +// rotation invalidation cost (Task #405). +// +// `enforce_scrollback_limit` (`resize_and_alt.rs`) now sets +// `self.merge_cache = None` every time a line feed's push+drain rotation +// happens with scrollback already at capacity (see +// `Buffer::merge_cache`'s field doc and the +// `incremental_merge_matches_oracle_after_scrollback_capacity_rotation` +// regression test in `flatten.rs`) — this is the correctness fix, but it +// also means the very next `visible_as_tchars_and_tags` flatten can no +// longer take the incremental fast path and must fully re-merge the whole +// visible window instead. This benchmark isolates that worst case: a +// buffer already sitting at its scrollback cap, then one LF (which always +// rotates) immediately followed by one flatten, repeated every iteration +// so the flatten never gets to reuse a warm cache. Two window heights are +// measured to show the cost scales with `height` (a full-window re-merge), +// not with `scrollback_limit`. +// --------------------------------------------------------------- +fn build_buffer_at_scrollback_capacity( + width: usize, + height: usize, + scrollback_limit: usize, +) -> Buffer { + let mut buf = Buffer::new(width, height).with_scrollback_limit(scrollback_limit); + // Fill well past capacity so scrollback is already at its steady-state + // cap before any timed iteration runs (pre-fill happens outside the + // timed closure via `iter_batched`'s setup callback). + let total_lines = height + scrollback_limit + 8; + for i in 0..total_lines { + let text: Vec = format!("line{i:06}") + .bytes() + .cycle() + .take(width) + .map(TChar::Ascii) + .collect(); + buf.insert_text(&text); + buf.handle_lf(); + buf.handle_cr(); + } + buf +} + +fn bench_lf_flatten_at_capacity(c: &mut Criterion) { + const WIDTH: usize = 200; + const SCROLLBACK_LIMIT: usize = 4_000; + + let mut group = c.benchmark_group("bench_lf_flatten_at_capacity"); + + for height in [24usize, 100usize] { + group.bench_with_input( + BenchmarkId::new("lf_then_flatten_steady_state", height), + &height, + |b, &height| { + // `iter_batched_ref` (not `iter_batched`): the routine takes + // `&mut Buffer`, so the ~`height + SCROLLBACK_LIMIT`-row + // buffer's destructor runs when the batch's input `Vec` is + // dropped — OUTSIDE the timed span — instead of inside the + // routine on every iteration. With by-value `iter_batched` + // the per-iteration buffer drop dominated the measurement + // (thousands of `Row` frees), swamping the single LF+flatten + // this bench isolates and making the result track + // `SCROLLBACK_LIMIT` rather than `height`. The buffer stays at + // capacity across the reused iterations (each LF is a + // push+drain that nets `rows.len()` unchanged), so every timed + // iteration remains in the exact steady state under test. + b.iter_batched_ref( + || { + let mut buf = + build_buffer_at_scrollback_capacity(WIDTH, height, SCROLLBACK_LIMIT); + // Warm the merge cache once, outside the timed + // section, so the timed rotation+flatten pair below + // starts from a populated (not merely absent) cache + // — matching the real steady-state PTY loop, where + // the previous frame already warmed it. + let _ = buf.visible_as_tchars_and_tags(0); + buf + }, + |buf| { + buf.insert_text(&[TChar::Ascii(b'x')]); + buf.handle_lf(); + buf.handle_cr(); + std::hint::black_box(buf.visible_as_tchars_and_tags(0)); + }, + BatchSize::SmallInput, + ); + }, + ); + } + + group.finish(); +} + // --------------------------------------------------------------- // Benchmark: erase display (ED) on a full buffer // --------------------------------------------------------------- @@ -1024,6 +1117,7 @@ criterion_group!( bench_cursor_ops, bench_move_cursor_relative, bench_lf_heavy, + bench_lf_flatten_at_capacity, bench_erase_display, bench_scrollback_render, bench_alternate_screen_switch, diff --git a/freminal-buffer/src/buffer/flatten.rs b/freminal-buffer/src/buffer/flatten.rs index e9b01c7d..a1b29f18 100644 --- a/freminal-buffer/src/buffer/flatten.rs +++ b/freminal-buffer/src/buffer/flatten.rs @@ -3101,6 +3101,98 @@ mod incremental_merge_tests { ); } + /// Task #405 regression guard: `Buffer::enforce_scrollback_limit` + /// (`resize_and_alt.rs`) is a fourth rotation site in the same family as + /// `scroll_slice_up`/`_down` and whole-buffer `scroll_up` proven above. + /// Once scrollback is at capacity, every LF pushes a new row at the + /// bottom and then drains `overflow` rows from the front + /// (`self.rows.drain(0..overflow)` + `self.row_cache.drain(0..overflow)`), + /// netting `rows.len()` unchanged. Since `visible_window_bounds` derives + /// purely from `rows.len()` and `height` (both unchanged), the resulting + /// `MergeWindowFp` is numerically IDENTICAL to the fingerprint the + /// previous call's `merge_cache` was built against, even though every + /// row's identity within that window just rotated down by `overflow` — + /// so `enforce_scrollback_limit` now contains its own explicit + /// `self.merge_cache = None;` at the rotation point, mirroring the other + /// three sites (see `Buffer::merge_cache`'s field doc). + /// + /// This test warms `merge_cache`, then drives many LFs past the + /// scrollback cap, re-flattening after every single one (so the + /// fingerprint never gets a chance to visibly change between + /// rotations) and checks each result against the independent oracle. It + /// is the regression guard for that fix: if the `merge_cache = None;` + /// line in `enforce_scrollback_limit` is ever removed or bypassed, this + /// test fails. + #[test] + fn incremental_merge_matches_oracle_after_scrollback_capacity_rotation() { + let width = 20; + let height = 5; + let scrollback_limit = 5; + let mut buf = Buffer::new(width, height).with_scrollback_limit(scrollback_limit); + + let total_lines = 60; + for i in 0..total_lines { + buf.insert_text(&text(&format!("line{i:04}"))); + buf.handle_lf(); + buf.handle_cr(); + + // Warm + immediately re-read the merge cache after every single + // line feed, so the fingerprint check in + // `rows_as_tchars_and_tags_incremental` is exercised on every + // rotation, not just after several have accumulated. + let actual = buf.visible_as_tchars_and_tags(0); + let oracle = independent_oracle(&mut buf); + assert_eq!( + actual, oracle, + "iteration {i}: flatten diverged from the full-merge oracle \ + after a scrollback-capacity push+drain rotation" + ); + } + + // Belt-and-suspenders: independently verify the visible window's + // content directly via `Buffer::extract_text`, which reads + // `self.rows` / `row_cells_for_read` directly and goes through + // NEITHER `row_cache` NOR `merge_cache` — a completely separate + // code path from the one exercised (and cross-checked against the + // oracle) above. + // + // The loop above calls `handle_lf()` after EVERY line, including the + // last (`i == total_lines - 1`). That final LF advances the cursor + // onto a fresh blank row — exactly as a real terminal does — so the + // buffer's tail is `line{total_lines-1}` followed by ONE trailing + // blank row. The visible window's bottom row is therefore that blank + // row, and each content row `k` above it holds + // `line{total_lines - (height - 1) + k}`. Deriving the expectation + // from that geometry (rather than a `total_lines - height + k` + // formula that ignores the trailing blank) keeps this check an + // independent oracle over `extract_text` without baking in an + // off-by-one. + let (visible_start, visible_end) = buf.visible_window_bounds(0, 0); + assert_eq!( + visible_end - visible_start, + height, + "sanity: window must be exactly `height` rows" + ); + let content_rows = height - 1; + for (k, absolute_row) in (visible_start..visible_end).enumerate() { + let actual_row_text = buf.extract_text(absolute_row, 0, absolute_row, width - 1); + let expected = if k < content_rows { + // Bottom-most content line is `line{total_lines-1}`, sitting + // one row above the trailing blank; walk upward from there. + let expected_line_index = total_lines - 1 - (content_rows - 1 - k); + format!("line{expected_line_index:04}") + } else { + // Final visible row is the blank row created by the loop's + // last `handle_lf()`. + String::new() + }; + assert_eq!( + actual_row_text, expected, + "row {absolute_row} (window position {k}): expected {expected:?} via extract_text" + ); + } + } + // ──────────────────────────────────────────────────────────────── // Property test // ──────────────────────────────────────────────────────────────── diff --git a/freminal-buffer/src/buffer/mod.rs b/freminal-buffer/src/buffer/mod.rs index 582c6b1a..542ed689 100644 --- a/freminal-buffer/src/buffer/mod.rs +++ b/freminal-buffer/src/buffer/mod.rs @@ -159,15 +159,20 @@ pub struct Buffer { /// was built from, without marking every affected row dirty or /// `None`: [`Buffer::full_reset`], [`Buffer::reflow_to_width`], /// [`Buffer::enter_alternate`], and [`Buffer::leave_alternate`]. - /// - **Confined in-place row rotation**, in `scroll.rs`: - /// `scroll_slice_up`, `scroll_slice_down`, and `scroll_up`. These + /// - **Confined in-place row rotation**: `scroll_slice_up`, + /// `scroll_slice_down`, and `scroll_up` (`scroll.rs`), and + /// `enforce_scrollback_limit` (`resize_and_alt.rs`). These /// relocate already-clean, non-`None` cache entries between row /// indices (a moved row keeps its cached representation, only its /// index changes) without marking the moved rows dirty, and - /// without changing `self.rows.len()` (a shift or a - /// `remove`+`push` nets to the same length) — so *neither* `fp` - /// *nor* `first_rebuilt_row` observes that row content moved to a - /// different index. Each of these three functions contains its own + /// without changing `self.rows.len()`: the three `scroll.rs` sites + /// net to the same length via a shift or a `remove`+`push`, and + /// `enforce_scrollback_limit` nets to the same length because its + /// front `drain` of `overflow` rows is paired, in the same line + /// feed, with a `push_row` of one new row at the bottom once + /// scrollback is at capacity — so *neither* `fp` *nor* + /// `first_rebuilt_row` observes that row content moved to a + /// different index. Each of these four functions contains its own /// `self.merge_cache = None;` at the point where the rotation /// happens; that line is load-bearing, **not** a redundant /// leftover safe to delete — removing it would let a cached @@ -175,7 +180,9 @@ pub struct Buffer { /// rotated indices. See [`flatten::MergeCache`]'s doc comment for /// the debug-only oracle cross-check that backstops this case, and /// `incremental_merge_tests` in `flatten.rs` for the regression - /// tests exercising it directly. + /// tests exercising it directly (including + /// `incremental_merge_matches_oracle_after_scrollback_capacity_rotation` + /// for the `enforce_scrollback_limit` site specifically). /// /// The `_columns` scroll variants (`scroll_slice_up_columns`/ /// `_down_columns`, used when DECLRMM confines a scroll horizontally) @@ -185,14 +192,18 @@ pub struct Buffer { /// an existing cache entry to a different index, so mechanism (2) /// already catches them. /// - /// Deliberately NOT invalidated here (relying on `fp` instead): the - /// row-count-changing sites in `scroll.rs` - /// (`enforce_scrollback_limit`'s and `erase_scrollback`'s front - /// `drain`s) and the row-append sites in `lines.rs`. Every one of - /// those changes `self.rows.len()` without a compensating removal - /// elsewhere, which changes the *absolute* `visible_start`/`visible_end` - /// bounds `visible_window_bounds` returns, so `fp` mismatches and a - /// full merge is forced. + /// Deliberately NOT invalidated here (relying on `fp` instead): + /// `erase_scrollback`'s front drain (`scroll.rs`) and the row-append + /// sites in `lines.rs`. `erase_scrollback` only runs its drain when + /// `visible_start > 0` and always collapses `visible_start` to `0` + /// (everything above the live view is discarded, not rotated in + /// place), so the *absolute* `visible_start`/`visible_end` bounds + /// `visible_window_bounds` returns always change — unlike + /// `enforce_scrollback_limit`, there is no case where the window + /// bounds net out unchanged. The `lines.rs` append sites only ever grow + /// `self.rows.len()` with no compensating removal, which likewise + /// always changes the window bounds. Both cases make `fp` mismatch, so + /// a full merge is forced without an explicit `None`. pub(in crate::buffer) merge_cache: Option, /// Width and height of the terminal grid. diff --git a/freminal-buffer/src/buffer/resize_and_alt.rs b/freminal-buffer/src/buffer/resize_and_alt.rs index a3194682..5a25d59c 100644 --- a/freminal-buffer/src/buffer/resize_and_alt.rs +++ b/freminal-buffer/src/buffer/resize_and_alt.rs @@ -1006,6 +1006,27 @@ impl Buffer { self.gc_unreferenced_blocks(); self.adjust_prompt_rows(overflow); + // Task #405 fix: once scrollback is at capacity, every line feed + // pushes one row at the bottom (via `push_row`/`handle_lf`) and this + // drain then removes `overflow` rows from the front in the same + // logical step, netting `self.rows.len()` unchanged. Because + // `visible_window_bounds` (and therefore `MergeWindowFp`) derives + // purely from `rows.len()` and `height` — both unchanged here — the + // fingerprint the next flatten computes is numerically IDENTICAL to + // the one the stale `merge_cache` was built against, even though + // every surviving row's identity just shifted down by `overflow`. + // This is the same confined-rotation bug class as `scroll_up` + // (`scroll.rs`, see its own `merge_cache = None` and the + // explanation in `Buffer::merge_cache`'s field doc): the fingerprint + // and `first_rebuilt_row` checks cannot observe an index shift that + // isn't accompanied by a `rows.len()` change or a dirty/`None` + // marking. Null the cache to force a full re-merge next flatten. + // + // Only reachable here, inside the `overflow > 0` branch — the + // early-return above (`self.rows.len() <= max_rows`) drains nothing + // and must not pay this cost on every line feed. + self.merge_cache = None; + // --- Garbage-collect images no longer referenced by any row --- if !self.image_store.is_empty() { self.image_store