perf: streaming highlight + perceived-latency fixes for large diffs (rebase of #288) - #394
Open
r-vdp wants to merge 4 commits into
Open
perf: streaming highlight + perceived-latency fixes for large diffs (rebase of #288)#394r-vdp wants to merge 4 commits into
r-vdp wants to merge 4 commits into
Conversation
The parse phase now emits highlight jobs instead of highlighting inline. A background worker drains them in parallel and streams results back to the main thread via an mpsc channel; the main loop patches spans into diff_files in place between frames. Effect: the file list and diff structure appear after the parse cost alone, instead of after parse + full synchronous highlight. Total work is unchanged (slightly more, from thread + channel overhead) but the visible-frame wait shrinks on big diffs. Notes worth recording: * git2 / hg / jj state is mostly !Send. Container-grammar full-file content is fetched on the main thread during parse and embedded into jobs as owned Strings; the worker never touches VCS state. * Workers use thread::scope + an atomic-cursor pulled from a shared Vec<HighlightJob>. Beats static chunking when one giant file sits alongside many small ones. * Per-hunk jobs and full-file (Vue/Svelte/...) jobs share a single HighlightJob enum and a single dispatch path. * HighlightSession bundles the receiver and a cancel AtomicBool. On diff swap (commit reselection, :e reload), the old session is dropped and its cancel flag set; the worker exits at its next channel send or cancel check. * Adaptive event-poll cadence: 30ms while a session is live so streamed spans land within a frame, 100ms idle to keep wakeups low. Tests use a synchronous streaming::run_blocking helper so spans are present deterministically; production code path is the worker.
Pending jobs now live in an Arc<Mutex<VecDeque<HighlightJob>>> instead
of an Arc<Vec> with an atomic cursor. The worker still pops jobs in
queue order, but the App can reorder the remaining queue by file-index
distance whenever the user moves between files.
Why: when the user clicks the last file in a 565-file commit, the
worker is still grinding through files near index 0 that aren't on
screen. The visible diff sits unhighlighted for several seconds while
the worker walks the queue. Sorting pending jobs by abs_diff from the
current file_idx puts visible work first.
Called from:
* jump_to_file (file list click, } / { navigation)
* update_current_file_from_cursor (cursor / scroll moved into a new file)
The sort is O(n log n) on the remaining queue, run under a mutex held
only long enough to reorder. Workers contend on the same mutex per
pop, but the lock is held for microseconds and navigation events are
human-paced, so contention is negligible in practice.
The main loop used to read one event and redraw, read one event and redraw. On a big diff where a single render takes ~10 ms, holding j or wheel-scrolling fast queues events faster than the loop can consume them, and the backlog shows up as 200-600 ms of input lag - the worst case being a direction reversal, where the user has to wait for the Up backlog to drain before the first Down is even read. Now after the blocking event::poll(poll_timeout), the loop keeps pulling events with event::poll(Duration::ZERO) until the queue is empty (or a 64-event cap is hit, in case of a paste or stuck key), then redraws once. The cap is a safety; in practice almost all bursts drain in well under 64 events. The existing 'continue' shortcuts inside the match (Ctrl+C, ZQ, dd, ;X, pending state setters) now use 'continue 'main' so they short- circuit straight to the outer loop's render path. Those keys aren't the high-throughput case batching is meant to help with, and keeping them per-event means each one renders before the next is read - which is what the user expects for atomic-action keys.
When the user wheel-scrolls the file list while the diff panel has focus, the render-time 'sync selection to diff's current file' path selects the diff's file, and ratatui's List widget then auto-adjusts the offset on render to keep that selection visible - undoing the wheel. Net effect: the file list stays glued to whichever file the diff cursor is on, even when the user is trying to scroll past it. Track an 'I have manually scrolled the file list' flag, set by file_list_viewport_scroll_up/down. While the flag is set, skip the render-time sync entirely, so the user's offset stays put. The flag is cleared whenever the diff's current_file_idx changes for a reason other than file-list wheel (jump_to_file, cursor scrolling into a different file via update_current_file_from_cursor), which is the natural moment to resume tracking. Pre-existing bug, surfaced now that the rest of the loop is fast enough that wheel-scrolling the file list is actually responsive enough to expose the snap-back.
agavra
reviewed
Jun 3, 2026
agavra
left a comment
agavra
left a comment
Owner
There was a problem hiding this comment.
Thanks for rebasing this @r-vdp - I'm curious whether this was something you needed in your local testing. I'm not eager to make the diff rendering a lot more complicated unless people are regularly viewing massive diffs. There are probably a lot more low-hanging fruits to do first before we implement streaming highlighting.
Contributor
Author
|
Yeah, I hit this when I was working on a remote box over ssh on a slow connection, going through diffs was painfully slow. I have some other, smaller diff, perf improvements sitting locally, I can re-order my patches and submit those instead if you're not eager on merging this one. |
Owner
|
Yeah let's start with those, I also bet that for slow connections the best thing we can do is auto-collapse some files similarly to how github does it for large diffs since it's mostly about transferring bytes rather than actual loading times (maybe? i'm not actually sure how SSH terminals work in detail). I know there's a lot of hype going on recently about Pierre's diffs being blazing fast, but I think that's more novelity than use and I imagine it can create a lot of bugs if we start doing things async. |
Contributor
Author
|
The main issue that I encountered was the frame-by-frame rendering being super slow. Every up or down scroll movement would take multiple seconds to rerender the viewport. I'll review my patch stack and see what else is upstreamable. |
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
3 participants
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.
This is #288 by @ptoussai rebased onto current main, since the original branch is still based on pre-v0.13.0 and no longer applies. Authorship is preserved on all four commits.
Supersedes #288.