Repository navigation
fix: bound hot-path costs found by a performance scan - #98
Merged
Merged
Conversation
Rows that trim to zero cells were charged 0 bytes and never filled a page, so a blank-line flood (yes '') grew history past scrollback-limit forever. Charge each row its RowMeta size and seal pages on a row-count cap too.
Each Redraw from an occluded pane looped refresh_window_title over every dirty occluded window, so K busy background tabs cost O(K^2) Terminal locks per second on the main thread. Only re-assert titles for windows leaving occlusion; still-occluded ones already refreshed on their own Redraw.
Remote panes posted one main-thread Redraw per WS frame with no floor; each locks the terminal and rebuilds the session card. Coalesce through the local path's decide_redraw_floor with a trailing deadline. An overtaken scrollback snapshot was refetched unconditionally, so a pane whose local history already covers the tail reconnected in a tight loop. Refetch at most once per generation.
…erable Under synchronized output the trailing redraw was anchored to the last redraw request, so it could land inside the render side's snapshot hold window and be swallowed, leaving the final batch unpainted. Anchor it to the latest batch again. Capping overtaken refetches at one also gave up on races a later fetch would win, leaving older history missing for the whole connection. Keep refetching, immediately once and then with exponential backoff (1s..30s).
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Merged
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.
A static performance scan found four places where cost grew without a bound on hot paths. This fixes them.
scrollback-limit. Rows that trim to zero cells were charged 0 bytes and never filled a page, soyes ''grew history and memory forever. Each row is now charged itsRowMetasize, and pages also seal on a row-count cap.decide_redraw_floor) with a trailing redraw.Reviewer notes:
feed_remote_bytesno longer requests a redraw;run_connecteddecides it viaremote_redraw_decision. Three existing tests asserted one redraw per fed batch; those assertions were removed because that is the behavior being changed.now + SYNCHRONIZED_OUTPUT_MAX_SUPPRESSION), as before this branch. Anchoring it to the last redraw request lets it land inside the render side's snapshot hold window, where it would be swallowed.PagedScrollback::rowlookup. Both need a reflow-path redesign and are left for a follow-up.Validation:
cargo test --workspace --exclude noa-pty --exclude noa-ipc: 2,543 passed, 0 failed. noa-ipc tests need loopback bind and noa-pty tests need openpty; both were unavailable in the sandbox, and neither crate is changed.cargo clippy --workspace --all-targetsshows no new warnings.rustfmt --checkon changed files andgit diff --checkpassed.Limitations: