Skip to content

perf: avoid redundant client ANSI encoding work - #4552

Merged
JJLiebig merged 3 commits into
masterfrom
issue/4546-client-ansi-performance
Sep 24, 2026
Merged

JJLiebig merged 3 commits into
masterfrom
issue/4546-client-ansi-performance

Conversation

@JJLiebig

@JJLiebig JJLiebig commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Reduce client ANSI encoding work without changing frame cadence or the wire protocol:

  • Validate patch overlap by sorting intervals instead of comparing every pair.
  • Reuse the previous cell style within each encoding pass, preserving the existing equality check for styles that encode identically.
  • Skip terminal writes for metadata-only patches with an unchanged hidden/absent cursor; cursor changes and visible-cursor suppression still take the normal path.

Measurements

Windows, Ryzen 7 9800X3D, 16 logical CPUs, optimized builds; medians of three paired runs in alternating order (30 samples per run after five warmups). Baseline 9903007; the subsequent master update only changes live-handoff test synchronization.

Encoder workload Before After
554×109, 20,165 fragmented runs 70.546 ms 0.971 ms
554×109, dense single-style redraw 19.064 ms 0.827 ms
120×40, dense redraw 1.042 ms 0.081 ms
Metadata-only patch, unchanged hidden/absent cursor 43–45 bytes 0 bytes

These are component elapsed times, not whole-process CPU savings. Single-cell spinner encoding did not improve (0.36 → 0.60 µs in the wide case). A separate five-run same-process stress test with a different style on every cell was ~7% slower (991 → 1,061 µs); repeated styles improved from 948 → 21 µs.

Visible-update replay captures were byte-identical in all six baseline/candidate runs. The large speedups reduce client work; they do not establish lower Ghostty/WindowServer CPU. Herdr already emits changed cell regions. The macOS CPU incident in #4546 has not been reproduced here, and this PR does not classify spinners or delay small updates.

Validation

  • Regression coverage for unsorted/overlapping/adjacent patch runs, style aliases and hyperlinks, metadata-only patches, cursor movement and suppression.
  • Investigation candidate: 226 focused release tests, six render-scale profiling tests, and six UI hot-path architecture tests passed.
  • Delivery checks and reviews are recorded below.

Delivery validation on e676547: just check passed (3,236 Rust tests; maintenance, architecture, integration and documentation checks; Clippy, formatting and dev build). Review Suite fast: both reviewers reported no findings (rvw_ad6ca41c). All eight PR checks passed, including Linux/macOS/Windows CI and Windows ConPTY packaging. Greptile approved this exact head (5/5); CodeRabbit completed its review with no actionable comments. Its generic docstring-coverage warning does not identify a behavior or documentation defect; no unrelated docstring expansion was added.

@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: d1c1e324-5033-4821-8775-b49adf55890e

📥 Commits

Reviewing files that changed from the base of the PR and between e676547 and 3b1fca1.

📒 Files selected for processing (1)
  • src/client/state.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/client/state.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Patch rendering now sorts row spans before checking for overlap and caches raw cell-style tuples during cell writes. When a patch has no changed rows and the cursor state is unchanged and absent or hidden, encoding returns no bytes. The client skips writing and flushing stdout when the encoded payload is empty. Tests cover overlap ordering, style output, and cursor-state cases.

Merge Risk: ⚪ Minimal · up to 3b1fc

No actionable merge-blocking issue is established; this change is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: reducing redundant client ANSI encoding work. It is concise and specific.
Description check ✅ Passed The description directly explains the ANSI encoding optimizations, behavior preservation, measurements, and validation for the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kangal-bot

Copy link
Copy Markdown
Collaborator

@coderabbitai review
@greptileai

@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

@kangal-bot I will review the changes in #4552.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@greptile-apps

greptile-apps Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge with no actionable correctness, security, or repository-rule issues identified.

Summary

This PR reduces client-side ANSI rendering work while preserving terminal output semantics.

  • Avoids writing and flushing stdout when a committed metadata-only patch produces no terminal bytes.
  • Replaces pairwise patch-overlap checks with sorted interval validation.
  • Reuses the previously processed cell style while preserving equality of equivalent SGR encodings.
  • Adds regression coverage for unsorted and adjacent runs, style aliases, hyperlinks, metadata-only updates, and cursor behavior.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  P[Surface patch] --> V[Validate row bounds and overlap]
  V --> E{Cells or cursor output needed?}
  E -->|No| C[Commit metadata-only patch]
  E -->|Yes| A[Encode ANSI using cached style]
  A --> W[Write and flush terminal bytes]
  W --> C
Loading

Reviews (2) · Last reviewed commit: "Merge branch 'master' into issue/4546-cl..."

@JJLiebig

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review
@greptileai

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@JJLiebig
JJLiebig merged commit afd9e19 into master Sep 24, 2026
11 of 12 checks passed
abhijit-s pushed a commit to abhijit-s/herdr that referenced this pull request Sep 25, 2026
Merges 9 upstream commits: native kitty graphics rendering replacing
the custom graphics API (herdrdev#4561, breaking), navigator search term
matching (herdrdev#4535), avoiding inferring codex idle from terminal output
(herdrdev#4563), Windows input coverage reporting and clipboard test fixes
(herdrdev#4566), qualified Win32/Kitty input path test defaults (herdrdev#4587),
reduced redundant client ANSI encoding work (herdrdev#4552), reduced repeated
scrollback page lookups (herdrdev#4553), tmux auto-attach guard docs (herdrdev#4592),
and keeping copy mode active during projection updates (herdrdev#4281).

refs upstream herdrdev/herdr@8d95e9bd
hdosys added a commit to hdosys/herdr-ext that referenced this pull request Oct 1, 2026
…ates

Adopt the stable cursor-settling sequence, row-based scrollback reads and redundant ANSI work reduction. Keep fork cursor color live during a position hold and do not skip metadata-only output when an OSC 12 color change or OSC 112 reset remains pending.

Upstream sources: 856b64b, d59d060, 21fd121, 8d64c34, afd9e19.

Refs herdrdev#4389, herdrdev#4404, herdrdev#4554, herdrdev#4553, herdrdev#4552, herdrdev#4303, herdrdev#4506, herdrdev#4546

Co-authored-by: JJ Liebig <jonathan.liebig@gmail.com>
maciejul pushed a commit to maciejul/herdr-supercharged that referenced this pull request Oct 28, 2026
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.

2 participants