fix(terminal): cap active block height and skip redundant SumTree rebuilds#9738
fix(terminal): cap active block height and skip redundant SumTree rebuilds#9738monotykamary wants to merge 4 commits intowarpdotdev:masterfrom
Conversation
…uilds When a long-running command (e.g. git clone) produces large amounts of output, the active block's displayed height grows unboundedly. Every frame, update_live_block_height() traverses the entire DisplayedOutput BiMap and rebuilds the block_heights SumTree, which is O(R) per frame where R can be 10,000+ rows. This causes FairMutex contention between the PTY reader thread and the UI thread, stalling the terminal. Add two feature flags behind which we gate the fixes: 1. CapActiveBlockHeight: caps the reported height of an executing block to viewport rows. The full output remains in FlatStorage for scrollback; only the SumTree entry is capped. When the block finishes, the final height is computed from full grid contents. 2. CompactProgressLines: caches the last computed active block height and skips the SumTree rebuild when it hasn't changed between frames. This handles the common case during CR-based progress output where carriage returns overwrite the same line in-place without adding rows. Also consolidates three separate FairMutex acquisitions in handle_terminal_wakeup() into a single lock scope, reducing contention with the PTY reader thread. Refs: warpdotdev#3626, warpdotdev#8409, warpdotdev#8205, warpdotdev#6743 Co-Authored-By: Warp <agent@warp.dev>
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @monotykamary on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
github-actions
Bot
added
the
external-contributor
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
…match The initial commit introduced two bugs caught by cargo check: 1. The BlockList constructor was missing its closing brace (}), breaking the struct literal and function scope. 2. update_live_block_height() was pushing Lines into BlockHeightItem::Block which expects BlockHeight(Lines). Also invalidates the last_active_block_height cache on resize, since the CapActiveBlockHeight cap depends on viewport rows and block heights change on reflow. Co-Authored-By: Warp <agent@warp.dev>
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and posted feedback on this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds feature-flagged terminal performance changes that cap active block height and skip redundant block height SumTree rebuilds, plus consolidates some terminal wakeup model locking.
Concerns
- The active block height cap updates only the SumTree entry while rendering and several viewport helpers continue to use the block's full height. That makes scroll/follow-bottom coordinates inconsistent while a command is still running, so a long active block can be treated as viewport-sized and clipped from the top instead of showing the newest output until it finishes.
- Supplemental security pass found no security-specific issues in the inlined diff.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
…oll math Capping the stored block height in the block_heights SumTree caused the viewport/scroll calculations (which use the SumTree total) to disagree with the renderer (which uses the block's actual grid height). Once output exceeded the viewport cap, follow-bottom treated the block as viewport-sized and the renderer clipped from the top — users would stop seeing new output until the command finished. Remove the cap from update_live_block_height() entirely. The CompactProgressLines cache (which skips the SumTree rebuild when the height hasn't changed between frames) is sufficient to solve the performance problem without breaking scroll/viewport math. Keep the CapActiveBlockHeight feature flag as a placeholder for future work that could apply the cap at the rendering/viewport layer instead of the stored height. Co-Authored-By: Warp <agent@warp.dev>
There was a problem hiding this comment.
Capping the stored block_heights SumTree height breaks the scroll/viewport math since block_list_viewport.rs uses block_heights().summary().height for scroll position calculations and follow-bottom anchoring. Once output exceeds the cap, the viewport treats the block as viewport-sized and new output becomes invisible.
Fixed in d6d3832: removed the CapActiveBlockHeight cap from update_live_block_height() entirely. The CompactProgressLines cache (which skips the SumTree rebuild when the height hasn't changed between frames) is sufficient to solve the performance problem without breaking scroll math. Kept the flag as a placeholder for future work that could apply the cap at the rendering/viewport layer instead of the stored height.
1 participant
Description
What
Two feature-flagged optimizations to fix terminal hangs under high-throughput output (e.g.
git clone):block_heightsSumTree rebuild when it hasn't changed between frames. Also consolidatesFairMutexacquisitions in the wakeup handler to reduce contention with the PTY reader thread.block_list_viewport.rsusesblock_heights().summary().heightfor scroll position and follow-bottom anchoring).Why
When a long-running command like
git cloneproduces large amounts of output, every frameupdate_live_block_height()callsblock.height()→output_grid_displayed_height(), then rebuilds theblock_heightsSumTree by slicing + recombining. This causesFairMutex<TerminalModel>contention between the PTY reader thread (which holds the lock while parsing up to 64KB of ANSI bytes) and the UI thread (which holds it during block height computation). When both compete, the PTY reader can block on the lock, causing kernel PTY backpressure on the child process — effectively stallinggit cloneentirely.This is the root cause behind the "screen rendering locks up under extreme stdout/stderr load" issue (#3626), as well as related reports of Warp hanging with Claude Code (#8409), extreme memory usage (#8205), and general slowness (#6743).
How
CompactProgressLines (feature flag, enabled for dogfood):
last_active_block_height: Option<Lines>cache toBlockList.update_live_block_height(), when the block is still executing and the computed height matches the cached value, early-return without rebuilding the SumTree.git cloneprogress bars) where\roverwrites the same line in-place without adding rows.handle_terminal_wakeup(), merged three separatemodel.lock()acquisitions into a single lock scope, reducing FairMutex contention windows.CapActiveBlockHeight (feature flag, placeholder — not currently wired):
block_heights().summary().height) to disagree with the renderer (which uses the block's actual grid height). Once output exceeds the cap, follow-bottom treats the block as viewport-sized and new output becomes invisible.Impact
git clone(CR-based progress output)Workarounds (without this fix)
git clone --no-progress ...— suppresses progress output entirelyGIT_PROGRESS_DELAY=9999 git clone ...— delays progress output until completionLinked Issue
ready-to-implement.bug,triaged,ready-to-implementScreenshots / Videos
N/A — performance fix with no visible UI changes. The behavioral change is that
git cloneand similar high-throughput commands no longer hang the terminal.Testing
cargo check --features cap_active_block_height,compact_progress_lines— passescargo fmt -- --check— passescargo clippy— no new warningscargo run --features compact_progress_lines, thengit clone https://github.com/torvalds/linux.git— completes without hangAgent Mode
Related upstream issues: #3626, #8409, #8205, #6743
CHANGELOG-BUG-FIX: Fixed terminal hangs under high-throughput output (e.g. git clone) by skipping redundant layout rebuilds and consolidating FairMutex acquisitions.