Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve conhost's scrolling performance #16333

Merged
merged 7 commits into from
Dec 5, 2023
Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Nov 17, 2023

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

  • typeing 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 ✅

@lhecker lhecker added Product-Conhost For issues in the Console codebase Area-Performance Performance-related issue zInbox-Bug Ignore me! labels Nov 17, 2023
@lhecker
Copy link
Member Author

lhecker commented Nov 17, 2023

Before:

20231117_161011.mp4

After:

20231117_161137.mp4

@DHowett
Copy link
Member

DHowett commented Nov 17, 2023

what on earth sorcery is this

DHowett
DHowett previously approved these changes Nov 17, 2023
@zadjii-msft
Copy link
Member

holy fuck what

zadjii-msft
zadjii-msft previously approved these changes Nov 27, 2023
@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 27, 2023
void UpdateScrollBars();
void InternalUpdateScrollBars();
ScrollBarState FetchScrollBarState();
Copy link
Member Author

@lhecker lhecker Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@zadjii-msft zadjii-msft dismissed stale reviews from DHowett and themself December 4, 2023 21:33

We had to re-write it

@DHowett DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 5, 2023
@DHowett DHowett disabled auto-merge December 5, 2023 02:02
@DHowett DHowett merged commit 71a6f26 into main Dec 5, 2023
@DHowett DHowett deleted the dev/lhecker/conhost-scroll-perf branch December 5, 2023 02:02
DHowett pushed a commit that referenced this pull request Jan 29, 2024
`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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Performance Performance-related issue Product-Conhost For issues in the Console codebase zInbox-Bug Ignore me!
Projects
Development

Successfully merging this pull request may close these issues.

3 participants