perf: skip geometry reconciliation on the text-input redraw - #226
Merged
Conversation
After every printable keystroke the keyDown path called forceRefresh, which reasserts the surface's display id and runs forceRefreshSurface()'s geometry reconciliation. Both exist for topology changes -- split close/reparent, and the stuck-vsync state after wake-from-sleep -- and a character cannot cause either. Typing changes grid contents, not the surface's size or which display it is on. Adds requestRedrawAfterInput(), which only calls ghostty_surface_refresh, and uses it at the text-input call site. forceRefresh is unchanged and still used everywhere else. The early-out conditions are deliberately identical to forceRefresh's, so when a redraw happens is unchanged; only the work done differs. This half was unmeasurable until now: the lag harness sends an arrow key, which never satisfies shouldRefreshAfterTextInput, so forceRefresh never ran under it. #216 added a lag_combo input so a printable key can drive the same harness through the real responder chain. Refs #183
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
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.
Closes the remaining half of #183. Measured with the in-process sampler, not the socket harness.
What this does
After every printable keystroke the keyDown path called
forceRefresh, which reasserts the surface's display id and runsforceRefreshSurface()'s geometry reconciliation. Both exist for topology changes — split close/reparent, and the stuck-vsync state after wake-from-sleep. A character causes neither: typing changes grid contents, not the surface's size or which display it lives on.Adds
requestRedrawAfterInput(), which only callsghostty_surface_refresh, and uses it at the text-input call site.forceRefreshis unchanged and still used everywhere else.Early-out conditions are deliberately identical to
forceRefresh's, so when a redraw happens is unchanged — only the work done differs.The numbers
Two before/after pairs, 400 keystrokes each, timed in-process at the call site:
forceRefreshrequestRedrawAfterInputComplete separation on every metric across both pairs — roughly 40-100x.
Honest about magnitude: the absolute saving is ~0.15-0.3 ms per keystroke. Not transformative on its own, but it is pure waste on the path this codebase treats as sacred, and removing it costs nothing.
Why measuring this took three attempts
The first attempt used
tests/test_workspace_churn_up_arrow_lag.pyand produced four runs of completely overlapping ranges. That harness measures a socket RPC round trip — IPC, JSON,DispatchQueue.main.sync, AppKit dispatch, then the work. The transport is order-milliseconds; the effect is ~0.3 ms. It was structurally incapable of resolving this, and more samples would not have helped.Hence #219 (the in-process sampler), #223 (the CI step that runs it) and #224 (its failure diagnostics). #225 fixed a crash those uncovered on the way — the typing profiler terminated the app whenever it was enabled, because
eventFieldsreadkeyCodeon non-key events.Test plan
xcodebuild -scheme programa→** BUILD SUCCEEDED **(verified from log contents)Branch is
perf/183-m2-fix-arather than something tidier: the original needed a rebase onto the crash fix, and force-push was correctly denied, so the same single commit went to a fresh name.