Improve conhost's scrolling performance#16333
Conversation
|
Before: 20231117_161011.mp4After: 20231117_161137.mp4 |
|
what on earth sorcery is this |
|
holy fuck what |
| }; | ||
| void UpdateScrollBars(); | ||
| void InternalUpdateScrollBars(); | ||
| ScrollBarState FetchScrollBarState(); |
There was a problem hiding this comment.
The problem with the tests was that briefly unlocking the console allowed window-resize messages to creep in.
To fix that, I had to do what I explained last week in our meeting: Instead of reaching through SCREEN_INFORMATION back into Window, I now leave control entirely inside Window. This allows me to properly unlock the console at the right time and also leave it unlocked.
This however made it necessary to move SCREEN_INFORMATION::ResizingWindow into Window, since the console lock is now not being held anymore. That's also why all the calls got changed from InternalUpdateScrollBars to just UpdateScrollBars. That is, now we're updating the scrollbars asynchronously during launch, etc. In my testing this works just fine.
This change is also another example for why I'm convinced that "loops" in control flow should be avoided (unless they're a good fit of course). Here, Window calls into SCREEN_INFORMATION and it calls back into Window with some extra data. That is very similar to our other callback-centric designs. I don't believe it's a coincidence that they had the same problems too. For instance TerminalInput accessing Terminal state without holding locks, is approximately similar to this code holding the console lock despite not needing it.
Basically I'm advocating for the latter in this graph, and I believe most of the former designs can be transformed to the latter, while retaining all the benefits:
sequenceDiagram
participant A as class A
participant B as class B
participant C as class C
opt A/B share control
activate A
A->>B: foo()
activate B
B->>C: bar()
C-->>B:
B-->>A:
deactivate B
deactivate A
end
opt only A is in control
activate A
A->>B: foo()
B-->>A:
deactivate A
activate A
A->>C: bar()
C-->>A:
deactivate A
end
We had to re-write it
`EnableScrollbar()` and especially `SetScrollInfo()` are prohibitively expensive functions nowadays. This improves throughput of good old `type` in cmd.exe by ~10x, by briefly releasing the console lock. ## Validation Steps Performed * `type`ing a file in `cmd` is as fast while the window is scrolling as it is while it isn't scrolling ✅ * Scrollbar pops in and out when scroll-forward is disabled ✅ (cherry picked from commit 71a6f26) Service-Card-Id: 91152166 Service-Version: 1.19
EnableScrollbar()and especiallySetScrollInfo()are prohibitivelyexpensive functions nowadays. This improves throughput of good old
typein cmd.exe by ~10x, by briefly releasing the console lock.Validation Steps Performed
typeing a file incmdis as fast while the window is scrollingas it is while it isn't scrolling ✅