perf: streaming highlight + perceived-latency fixes for large diffs - #288
perf: streaming highlight + perceived-latency fixes for large diffs#288ptoussai 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.
|
@ptoussai I'm not sure I want to add concurrency to tuicr in order to serve use cases with 47k line PR reviews. I'm highly suspect anyone is reviewing a PR that large by hand anyway, and this makes the potential for bugs significant. In my experience, loading even 10-20k line rust change is pretty speedy. Maybe because we don't need the full file highlighter the way I'd rather just default to collapsing large file diffs the way github does at some point. |
|
The change in question did not have any vue files actually. |
|
Btw, I’m only using this change as an easy way to test the perf myself, as it’s not easily tested in a headless way. A worst case scenario for sure, but also indicative that smaller changes will have less than ideal performance |
I agree, which is why I suggested things like collapsing large file diffs by default (like GitHub behavior). I'm curious if were you running it in release mode or debug mode? It makes a big difference when you run with
I admit I didn't fully think that through, what got me thinking about that was testing this PR locally and noticing it "flash" as things were being processed. So the initial load looked different than the final load.
I'd rather start with the simplest things just to make sure the tool doesn't crash before we get carried away with fancy perf optimizations. Not sure lazy/smart loading is any better than concurrency. I think if we show that there really is a need for faster loading then I'm ok with having a concurrent highlighting job but I'd rather it be blocking on render instead of streaming results back. |
|
That seems reasonable. The perf issue was not due to debug mode or container (vue etc) files but maybe something else is going on with that change. |
2 participants
Four perf/UX fixes that surfaced together while reviewing a 565-file commit (~47k lines, mostly JS/Vue/TS) in a large private repo. tuicr was slow to load, sluggish when switching files, and scrolling was effectively unresponsive (had to kill the process).
Each commit handles one concern. Per-commit messages have the details; quick recap:
perf(syntax): stream syntax highlighting on a background worker— parse emits jobs, a background worker highlights them in parallel and streams spans via mpsc; the main loop patchesdiff_filesbetween frames. File list + first diff structure appear after parse alone instead of parse + full sync highlight. Worker spawns lazily on the first drain soApp::new's post-install_diffreorderings (commit-message insert, directory sort) settle before jobs are resolved againstdiff_filesbysyntax_path.perf(syntax): prioritize highlight jobs around the focused file— pending jobs live in anArc<Mutex<VecDeque>>. When the user jumps files, the App reorders the remaining queue by file-index distance so visible work runs next instead of grinding files near index 0 that aren't on screen.perf(input): drain queued events before each redraw— the loop used to read one event then redraw; on a heavy diff, holdingjor wheel-scrolling queued 200-600ms of input lag, worst case on direction reversals. Now drains viaevent::poll(Duration::ZERO)after the blocking poll (64-event cap) then redraws once. Atomic-action keys (Ctrl+C, ZQ, dd,;X) keep their per-eventcontinue 'mainshortcut.fix(ui): preserve manual file-list scroll when diff panel is focused— wheel-scrolling the file list was snapping back because the render-time "sync selection to diff's current file" path re-selected each frame and ratatui'sListauto-adjusted offset to keep selection visible. Track amanual_file_list_scrollflag, set on wheel and cleared whencurrent_file_idxchanges for any other reason. Pre-existing; visible only once the rest of the loop is fast enough.