Skip to content

fix(renderer): repaint unchanged rows inside hard-scrolled regions - #143

Open
dmccaffery wants to merge 1 commit into
charmbracelet:mainfrom
dmccaffery:fix/stale-rows
Open

fix(renderer): repaint unchanged rows inside hard-scrolled regions#143
dmccaffery wants to merge 1 commit into
charmbracelet:mainfrom
dmccaffery:fix/stale-rows

Conversation

@dmccaffery

Copy link
Copy Markdown

After a successful hardware scroll, scrolln shifted the model of the physical screen on curbuf only, while the repaint loop in Render skips lines by newbuf.Touched. A row inside the scrolled region whose newbuf content is identical to the previous frame at the same index was therefore never repainted and kept displaying the scrolled-in content, leaving stale duplicate rows on screen.

Touch the scrolled region on newbuf as well, so every row whose physical cells moved is re-diffed against curbuf. Rows that already match emit nothing, so the optimization stays intact: benchmarks show no measurable regression (sec/op and bytes/frame unchanged within noise on Apple M2, benchstat n=10).

Adds a regression test asserting that curbuf matches the rendered frame after every render, plus scroll-path benchmarks for the unique- and repeated-row cases.

Benchmark results — issue #137 fix (touchLine(newbuf, …) in scrolln)

Renderer hard-scroll benchmarks (BenchmarkRendererScroll*, new in terminal_renderer_hardscroll_test.go):
an 80×24 fullscreen viewport with scroll optimization enabled, scrolled up one line per frame.
Compared with benchstat, n=10 runs each. goos: darwin, goarch: arm64, cpu: Apple M2.

Time per frame (sec/op)

Benchmark Before After Delta p-value
RendererScrollUniqueRows 92.82µ ± 3% 91.41µ ± 3% −1.51% p=0.035
RendererScrollRepeatedRows 97.67µ ± 1% 97.63µ ± 1% ~ p=1.000 (n.s.)
geomean 95.21µ 94.47µ −0.78%

Bytes written to the terminal (bytes/frame)

Benchmark Before After Delta p-value
RendererScrollUniqueRows 56.27 ± 0% 56.26 ± 0% ~ p=0.650 (n.s.)
RendererScrollRepeatedRows 97.22 ± 0% 96.05 ± 0% −1.20% p=0.000
geomean 73.96 73.51 −0.61%

Allocations (B/op)

Benchmark Before After Delta p-value
RendererScrollUniqueRows 9.562Ki ± 0% 9.562Ki ± 0% ~ p=1.000 (n.s.)
RendererScrollRepeatedRows 6.234Ki ± 0% 6.216Ki ± 0% −0.28% p=0.000
geomean 7.721Ki 7.710Ki −0.14%

Allocations (allocs/op)

Benchmark Before After Delta p-value
RendererScrollUniqueRows 1.405k ± 0% 1.405k ± 0% ~ p=1.000 (n.s.)
RendererScrollRepeatedRows 457.0 ± 0% 453.5 ± 0% −0.77% p=0.000
geomean 801.3 798.2 −0.38%

Summary

No performance regression from the fix. Touched-but-unchanged rows re-diff against curbuf and emit
nothing, so time per frame is unchanged within noise. The repeated-rows case (the bug scenario) writes
slightly fewer bytes after the fix because keeping curbuf accurate avoids spurious repaints on
later frames. Correctness: the issue's fuzz harness went from 5/38 corrupted trials to 0/4143.

Fixes: #137

  • I have read CONTRIBUTING.md.
  • I have created a discussion that was approved by a maintainer (for new features).

After a successful hardware scroll, scrolln shifted the model of the
physical screen on curbuf only, while the repaint loop in Render skips
lines by newbuf.Touched. A row inside the scrolled region whose newbuf
content is identical to the previous frame at the same index was
therefore never repainted and kept displaying the scrolled-in content,
leaving stale duplicate rows on screen.

Touch the scrolled region on newbuf as well, so every row whose
physical cells moved is re-diffed against curbuf. Rows that already
match emit nothing, so the optimization stays intact: benchmarks show
no measurable regression (sec/op and bytes/frame unchanged within
noise on Apple M2, benchstat n=10).

Adds a regression test asserting that curbuf matches the rendered
frame after every render, plus scroll-path benchmarks for the unique-
and repeated-row cases.

Fixes: charmbracelet#137
Signed-off-by: Deavon M. McCaffery <dmccaffery@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hard-scroll optimization leaves stale rows when the scrolled region contains lines unchanged since the previous frame

2 participants