From 9bb070be76e1f3048b97bae54d7bccdbe8ed70b7 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 17 Jun 2026 07:41:26 +0200 Subject: [PATCH] Session notes: the context-change flicker, pinned and fixed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updates §20.5 with the pinned cause (the restore scan ran after the swap, so the new content was briefly shown at the stale scroll) and the fix, plus productionization notes: the resolve-then-swap ordering is an invariant of the restore mechanism, the irreducible post-swap mapping window, and that fast renders unmask latent transients slowness was hiding. Co-Authored-By: Claude Opus 4.8 (1M context) --- focused-main-view-notes.md | 88 +++++++++++++++++++++++++------------- 1 file changed, 58 insertions(+), 30 deletions(-) diff --git a/focused-main-view-notes.md b/focused-main-view-notes.md index 26f9b346f..e873f2009 100644 --- a/focused-main-view-notes.md +++ b/focused-main-view-notes.md @@ -1875,36 +1875,64 @@ section once* (prep), *Add gocui accessor for the newly-loaded rows of an off-screen render* (prep), *Resolve diff lines in one batch pass instead of once per line* (the fix). (Plus the two middle-line commits.) -### 20.5 Follow-up surfaced by the speedup: a context-change flicker (no pager) +### 20.5 Follow-up surfaced by the speedup: a context-change flicker (no pager) — FIXED -The user then noticed: changing `-U` context with **no pager** while scrolled -down in the long diff shows a **pronounced flicker — a brief frame at a -different scroll position, random run-to-run**. **Not** reproducible with delta. +Changing `-U` context with **no pager** while scrolled down in the long diff +showed a **pronounced flicker — a brief frame at a different scroll position, +random run-to-run**; **not** reproducible with delta. The speedup didn't cause +it — it *unmasked* it: the old O(n²) scan throttled the read loop so the load +took ~33s, during which the old content sat stably on screen and hid the +transient; once the load was instant, the transient stood out. -Analysis so far (not yet pinned — avoid a third §11-style misdiagnosis): +**Pinned with the §10 `SetOriginY` tracer, at normal speed** (slow render would +have hidden it — it re-stretches the load to a stable old-content frame). The +log showed, every keypress, `SwapIn` then `SetOriginY` **~35ms later**: -- **The single-render path is clean.** `ContextLinesController.applyChange` does - one `HandleRenderToMain`; `linesToReadFromCmdTask` doesn't touch the origin; - the off-screen render keeps the *previous* content displayed at scroll S until - the EOF swap, at which point `Apply` sets the preserved origin in the same step. - For no-pager, `FirstPaintReady` never fires (no per-row backend), so the swap is - at EOF — unchanged by the perf fix. -- **The speedup unmasked it.** Before, the O(n²) scan throttled the read loop, so - the off-screen load took ~33s during which the old content sat *stably* at S — - hiding any startup/overlap transient. Now the load is instant, so a transient - frame around task start or the swap is suddenly visible. -- **no-pager vs delta is the cmd-vs-pty tell** (§13.1): no-pager takes the - *synchronous* `newCmdTask`; delta takes `newPtyTask`, which defers task creation - to `afterLayout`. So the suspect is something the synchronous path does (or - allows to overlap) that the deferred path coalesces away. -- **Suspects, unconfirmed:** (a) the 200ms loading-indicator goroutine firing - `beforeStart()` (`view.Reset()`) — but that flashes *blank* ("loading..." sits - above the viewport at scroll S), which doesn't match "a scroll position"; (b) a - second render overlapping the first (cf. §13.6's escape↔refresh overlap), the - better fit for a *random* scroll position. -- **Next step:** the §10 `SetOriginY` chokepoint tracer (`debugMainOriginReset`), - run at **normal speed** — `LAZYGIT_SLOW_RENDER` would *hide* this (it re-stretches - the load back out to a stable old-content frame). The log of origin writes + - callers during the flicker will pin which interleaving draws the bad frame. - Then fix per [[focused-main-view-flicker-timing-races]] (a known race gets a - real fix, not "live with it"). +``` +.647 SwapIn new content displayed, oy still 10354 (the old scroll) +.682 SetOriginY ->9865 Apply finally sets the preserved origin +``` + +**Cause:** `firstPaint` swapped the off-screen content in *then* ran +`RenderRestore.Apply`, which for the buffer-parse backend re-scans the whole +diff (the O(n) `resolveDiffLines`, tens of ms on a big diff) to find the line to +land on. For that window the new content was displayed at the **stale** +(now out-of-range) scroll; a layout draw landing there drew the bad frame. Delta +didn't flicker because its metadata/hyperlink backend resolves the target +*during* the load (`primaryBufferLine` set), so its `Apply` is instant — no scan +after the swap. (My initial guesses — the loading-ticker `Reset`, or a render +overlap — were both wrong; the tracer is what kept this from being a third +§11-style misdiagnosis. The single-render path *was* clean; the bug was the scan +on the wrong side of the swap.) + +**Fix (commit *Scan for the restore target before revealing the new content*):** +`RenderRestore.Apply` now takes a `swapIn func()` and owns the swap — it resolves +the target against the still-**off-screen** (and, at EOF, complete) buffer +*first*, then calls `swapIn`, then settles the scroll. The scan runs while the +previous content is still displayed, so the new content is revealed already at +the right position — the same flicker-free shape the early-resolving backends +always had. + +#### Productionization notes (keep in mind) + +- **The resolve-then-swap ordering is a real invariant of the restore mechanism, + not an incidental tweak.** When the gocui async-render improvements are + productionized (§15 decomposition (a)), `RenderRestore.Apply` must keep doing + its target resolution against the off-screen buffer *before* swapping in. + Reordering back to swap-then-resolve reintroduces the flicker for any backend + whose resolution isn't instant (i.e. buffer-parse / no-pager). The read-loop + unit test `TestNewCmdTaskRestore` guards this (asserts the render is *not* + swapped in before `Apply` runs). +- **A small post-swap window is irreducible in this design.** Mapping a matched + buffer line to a view line (`ViewLineForBufferLine`) needs the wrapped view + lines, which only exist after the swap, so the `swapIn → SetOrigin` step is + necessarily post-swap (~1ms). Delta has the same window and it's imperceptible; + not worth chasing to zero (would need swap+map+SetOrigin under one lock). +- **General lesson for the async-render PR: making renders fast unmasks latent + ordering/timing transients that slowness was hiding.** Re-test the restore + consumers (escape, `-U`, pager-switch) at **normal speed** after any perf work; + `LAZYGIT_SLOW_RENDER` *hides* this whole class by stretching the load back out. +- The batch `resolveDiffLines` is O(n) and "good enough" (user-confirmed), but the + §14.5/§16.4 *pinned-backend / incremental* optimization is still available if a + very large changed diff ever lags: today it parses every file section even when + metadata would answer, and re-snapshots per restore.