perf: minimize cursor::position queries#1090
Conversation
| prompt_start_row: u16, | ||
| prompt_start_row_stale: bool, |
There was a problem hiding this comment.
| prompt_start_row: u16, | |
| prompt_start_row_stale: bool, | |
| prompt_start_row: Option<u16>, |
Using Option instead of two separate values that are semantically linked would be better. Having None instead of checking a _stale field ensures it's not possible to accidentally use a stale value.
edit: Looks like there is at least one place where the stale value can be useful. In that case a custom enum would be better. Maybe something like:
enum CachedValue<T> {
Clear,
Stale(T),
Fresh(T),
}There was a problem hiding this comment.
Thanks for the feedback, @Bahex. I'd considered an enum but wanted to keep the change simpler; since you're also thinking it would be an improvement, I've pushed changes to combine the fields.
I went with a custom enum so I could have some convenience helpers implemented on it, but I'm also open to a generic CachedValue<T> like you suggest above if that's what you'd recommend/prefer.
If you have time to look at the updated version, please let me know what you think.
`Painter::repaint_buffer` issued a `cursor::position()` (CSI 6n / DSR)
on every paint to check whether the cached `prompt_start_row` had
drifted from the terminal's actual cursor row. Each query is a
synchronous round-trip — expensive over high-latency transports like
SSH.
Within a single `read_line` cycle reedline is the sole writer to the
terminal and updates `prompt_start_row` itself as its rendering
scrolls, so the drift check is unnecessary in the steady state.
Track that with `prompt_start_row_stale: bool` on `Painter`:
- Cleared in `initialize_prompt_position` (right after the cursor
query whose result we just stored).
- Set by any path that lets something else move the cursor or
writes content the painter does not model: `handle_resize`,
`print_external_message`, and `Reedline::open_editor`.
`paint_line` also invalidates defensively (its current callers
only run between `read_line` cycles, but the painter shouldn't
trust a cache after content it didn't track).
- When stale, the drift check queries the terminal and — on a
successful query that confirms no drift — clears the flag so
later paints in the same cycle skip the round-trip too.
The only remaining unavoidable DSR is in `initialize_prompt_position`
(reedline genuinely does not know where the host left the cursor
before `read_line` is called).
Maintenance contract: any new method that lets something else move
the cursor or writes content reedline does not account for in
`prompt_start_row` must call `Painter::invalidate_prompt_start_row`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
72b62b1 to
e383f7e
Compare
e383f7e to
0517b7c
Compare
Skip redundant cursor::position queries in the repaint hot path
Reedline issues a CSI 6n / DSR (
cursor::position()) on every paint to check whetherprompt_start_rowhas drifted from the terminal's real cursor row. That's a synchronous round-trip; over high-latency transports like SSH it dominates prompt-redraw latency.Within a
read_linecycle reedline is the sole writer to the terminal and updatesprompt_start_rowitself as its rendering scrolls — so the drift check is unnecessary in the steady state.A new
prompt_start_row_stale: boolonPaintertracks freshness:initialize_prompt_position.handle_resize,print_external_message,paint_line,open_editor).The hot-path drift check skips the DSR when the cache is fresh. The change is wholly inside reedline, so the improvement carries to any consumer (e.g., Nushell, brush, custom REPLs) without changes on their side.
Notes
I know that this is an area that may be challenging to test/validate. I'm open to any suggestions/feedback on how to ensure that this change does not regress existing functionality, while still improving the performance.
Full disclosure: as you can probably tell, I used Claude to identify and develop changes -- but I was heavily steering, and ultimately rewrote the code several times to address concerns that I had myself over maintainability challenges. I'm also placing this in draft first, with the hope to get early feedback on the approach and the maintainers' openness to considering this change. Any and all feedback is very much welcome, please 😄
Tradeoffs
cursor::position()fails see the same behavior as before: the drift check still runs and fails, with no optimization gain (and no regression).Measurements
Nushell 0.113.0 (HEAD) built against this branch's reedline vs against reedline
main. PTY harness (portable-pty), 80×24, scripted 80 ms input pacing, 5 Enter presses per run,--no-config-file --no-history. Three samples per condition; the harness is deterministic and produced identical DSR counts across samples within each condition.DSR counts:
mainSame harness with
--simulated-rtt-ms 125(each CSI 6n reply delayed 125 ms — comparable to a typical cross-region SSH round trip):mainThe remaining ~1 DSR/cycle on this branch is the unavoidable one in
initialize_prompt_position; reedline genuinely doesn't know where the host left the cursor beforeread_lineis called.brush, a smaller
reedline-based shell, shows the same shape (3.0 → 1.0 DSRs/cycle) — confirming the win is at the reedline layer, not shell-specific.