Skip to content

#104 #98 sanitize_output returns Cow + VecDeque grid for O(1) scroll#114

Merged
kirich1409 merged 1 commit intomainfrom
fix/104-cow-sanitize
Apr 14, 2026
Merged

#104 #98 sanitize_output returns Cow + VecDeque grid for O(1) scroll#114
kirich1409 merged 1 commit intomainfrom
fix/104-cow-sanitize

Conversation

@kirich1409
Copy link
Copy Markdown
Contributor

@kirich1409 kirich1409 commented Apr 14, 2026

Summary

  • Session read_loop: two allocations per PTY read — use Cow for sanitize #104 (Cow): `sanitize_output` allocated a `Vec` on every PTY read even when nothing was removed (no OSC-52 sequences). Changed return type to `Cow<'_, [u8]>`: fast path borrows the input slice (zero allocation), slow path (OSC-52 present) returns `Cow::Owned`. `session.rs` updated: `Bytes::from(sanitized.into_owned())` avoids the second copy.
  • VT parser: Vec::remove(0) in scroll_up is O(n) on hot path #98 (VecDeque): `ScreenBuffer::grid` was `Vec`, making `scroll_up` O(n) (`remove(0)` shifts all rows left) and `scroll_down` O(n) (`insert(0, ...)`). Changed `grid` to `VecDeque` — `pop_front`/`push_back` and `push_front`/`pop_back` are now O(1).

Acceptance Criteria

#104

  • `sanitize_output` returns `Cow<'_, [u8]>`
  • No allocation when sanitize is a no-op (test asserts `Cow::Borrowed`)
  • Sanitize tests pass

#98

  • `ScreenBuffer::grid` is `VecDeque`
  • `scroll_up` uses `pop_front` / `push_back` (O(1))
  • `scroll_down` uses `pop_back` / `push_front` (O(1))
  • `insert_lines`, `delete_lines`, `resize` updated accordingly
  • All 179 tests pass

Closes #104
Closes #98

Copilot AI review requested due to automatic review settings April 14, 2026 18:41
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR primarily targets performance on the PTY read hot path by making sanitize_output allocation-free when it’s a no-op, and updates session broadcasting to avoid an extra copy when sanitization produces an owned buffer. It also includes additional performance/test refactors in the REST layer and VT screen buffer.

Changes:

  • Change vt_parser::sanitize_output to return Cow<[u8]> with a borrowed fast path and owned slow path, plus tests asserting the borrowed behavior.
  • Update TerminalSession broadcasting to use Bytes::from(sanitized.into_owned()) to avoid a second copy when sanitization already allocated.
  • Add Store::for_tests() and refactor multiple REST-related tests to use it; introduce cached system metrics (SysCache) in REST state.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
runner/src/vt_parser.rs sanitize_output now returns Cow; additional ScreenBuffer storage/scrolling changes.
runner/src/session.rs Broadcast path updated to leverage Cow and avoid extra allocations/copies.
runner/src/store.rs Adds Store::for_tests() helper for in-memory SQLite + migrations.
runner/src/rest_handlers/sessions.rs Test helpers updated to use Store::for_tests() and new AppState.sys_cache.
runner/src/rest_api.rs Adds SysCache with TTL and threads it through AppState; refactors health/metrics to use the cache; test updates.
runner/src/main.rs Wires sys_cache into the production AppState initialization.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread runner/src/rest_api.rs
Comment thread runner/src/rest_api.rs
Comment thread runner/src/vt_parser.rs
Comment thread runner/src/vt_parser.rs
Comment thread runner/src/rest_api.rs
@kirich1409 kirich1409 changed the title #104 sanitize_output returns Cow to avoid allocation on hot path #104 #98 sanitize_output returns Cow + VecDeque grid for O(1) scroll Apr 14, 2026
@kirich1409 kirich1409 force-pushed the fix/104-cow-sanitize branch from 2aefd5d to 77b7fd5 Compare April 14, 2026 18:55
Closes #104

In the common case (no OSC-52 sequences) sanitize_output was allocating
a Vec<u8> for every PTY read even though it returned data identical to
the input. Changed return type to Cow<'_, [u8]>: borrows the input when
nothing is removed, allocates only when an OSC-52 sequence is stripped.

Also removes one extra copy in session.rs: Bytes::from(owned_vec) for
the sanitized case vs Bytes::copy_from_slice for the no-op case.
@kirich1409 kirich1409 force-pushed the fix/104-cow-sanitize branch from 77b7fd5 to 682ed72 Compare April 14, 2026 19:01
@kirich1409 kirich1409 merged commit 0aa4271 into main Apr 14, 2026
10 checks passed
@kirich1409 kirich1409 deleted the fix/104-cow-sanitize branch April 14, 2026 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Session read_loop: two allocations per PTY read — use Cow for sanitize VT parser: Vec::remove(0) in scroll_up is O(n) on hot path

2 participants