starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-03 21:25:34 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

[PR #4608] Throttle scrollbar updates in TermControl to ~one per 8ms #25859

New Issue
Open
opened 2026-01-31 09:12:16 +00:00 by claunia · 0 comments
claunia commented 2026-01-31 09:12:16 +00:00
Owner
Copy Link

Original Pull Request: https://github.com/microsoft/terminal/pull/4608

State: closed
Merged: Yes

Summary of the Pull Request

Redo #3531 but without the bug that it caused (#3622) which is why it was reverted.

I'm sorry if I explain this badly. If you don't understand a part, make sure to let me know and I will explain it better.

References

Will this reintroduce a crash?

I don't really understand that second check:
https://github.com/microsoft/terminal/pull/3256/files#diff-37b8cdfbaef0c1f807e324f0e6efb4edR1387-R1393
For now I removed it because it seems useless to me and it would make the code more complex.

Indeed, the scroll bar is held by the TermControl so if even if _closing is true, it should still be alive if this is alive. And if this is not alive, well referencing _closing will crash so that check doesn't do better here. So what is it for?

I cannot reproduce the crash from the issue that that PR fixed (#2248). I think that this is thanks to the weakThis check (when I remove it, it crashes). So I assume it's fine.

But I need help to know if I should add it back. @mcpiroman what do you think? Do you know if that check actually was useful or if can I remove it?

I'm leaving it as a draft PR until I solve this.

EDIT: see discussion below.

PR Checklist

  • Closes Add an ability to reset zoom (#3622)
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Explanation

How it worked before:
Terminal signals that viewport changed -> TermControl::_TerminalScrollPositionChanged gets called on the terminal thread -> it dispatches work for later to be ran the UI thread to updates the scrollbar's values

Why it's bad:

  • If we have many viewport changes, it will create a long stack of operations to run. Instead, we should just update the scroll bar with the most recent information that we know.
  • Imagine if the rate that the work gets pushed on the UI thread is greater than the rate that it can handle: it might freeze?
  • No need to be real time, we can wait just a little bit (8ms) to accumulate viewport changes before we actually change the scroll bar's value because it appears to be expensive (see perf below).

Now:
Terminal signals that viewport changed -> TermControl::_TerminalScrollPositionChanged gets called on the terminal thread -> it tells the ScrollBarUpdater about a new update -> the ScrollBarUpdater only runs one job (I don't know if that's the right term) on the UI thread at a time. If a job is already running but hasn't updated the scroll bar yet, it changes the setting in the already existing job to update the scroll bar with the new values. A job "waits" some time before doing the update to throttle updates because we don't need real time scroll bar updates. -> eventually, it updates the scroll bar
If the user scrolls when a scroll bar update is pending, we keep the scroll bar's Maximum and Minimum but let the user choose its new Value with the CancelPendingValueChange method.

Note

Also I changed a little bit the code from the Terminal to notify the TermControl less often when possible (686a7b362ccf2761c9531600c356317a625d9794). I can remove that commit if this is not in scope of this PR.

Performance

I'm a noob at using the tools to profile an application. Hopefully this is good enough. Perf in release mode while running 1..2000000 in Powershell during the first 4 seconds, without the last commit (686a7b362ccf2761c9531600c356317a625d9794):

Before:
before

After:
after

Validation Steps Performed

I tried to scroll with the scroll bar, with the mouse wheel. I tried to scroll while content is being outputted.

I tried to reproduce the crash from #2248 without success (good).

**Original Pull Request:** https://github.com/microsoft/terminal/pull/4608 **State:** closed **Merged:** Yes --- <!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request Redo #3531 but without the bug that it caused (#3622) which is why it was reverted. I'm sorry if I explain this badly. If you don't understand a part, make sure to let me know and I will explain it better. <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> ## References ### Will this reintroduce a crash? I don't really understand that second check: https://github.com/microsoft/terminal/pull/3256/files#diff-37b8cdfbaef0c1f807e324f0e6efb4edR1387-R1393 For now I removed it because it seems useless to me and it would make the code more complex. Indeed, the scroll bar is held by the `TermControl` so if even if `_closing` is `true`, it should still be alive if `this` is alive. And if `this` is not alive, well referencing `_closing` will crash so that check doesn't do better here. So what is it for? I cannot reproduce the crash from the issue that that PR fixed (#2248). I think that this is thanks to the `weakThis` check (when I remove it, it crashes). So I assume it's fine. But I need help to know if I should add it back. @mcpiroman what do you think? Do you know if that check actually was useful or if can I remove it? I'm leaving it as a draft PR until I solve this. **EDIT:** see discussion below. <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [x] Closes #3622 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Requires documentation to be updated * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx <!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments ### Explanation How it worked before: `Terminal` signals that viewport changed -> `TermControl::_TerminalScrollPositionChanged` gets called on the terminal thread -> it dispatches work for later to be ran the UI thread to updates the scrollbar's values Why it's bad: * If we have many viewport changes, it will create a long stack of operations to run. Instead, we should just update the scroll bar with the most recent information that we know. * Imagine if the rate that the work gets pushed on the UI thread is greater than the rate that it can handle: it might freeze? * No need to be real time, we can wait just a little bit (8ms) to accumulate viewport changes before we actually change the scroll bar's value because it appears to be expensive (see perf below). Now: `Terminal` signals that viewport changed -> `TermControl::_TerminalScrollPositionChanged` gets called on the terminal thread -> it tells the `ScrollBarUpdater` about a new update -> the `ScrollBarUpdater` only runs one job (I don't know if that's the right term) on the UI thread at a time. If a job is already running but hasn't updated the scroll bar yet, it changes the setting in the already existing job to update the scroll bar with the new values. A job "waits" some time before doing the update to throttle updates because we don't need real time scroll bar updates. -> eventually, it updates the scroll bar If the user scrolls when a scroll bar update is pending, we keep the scroll bar's Maximum and Minimum but let the user choose its new Value with the `CancelPendingValueChange` method. ### Note Also I changed a little bit the code from the Terminal to notify the TermControl less often when possible (686a7b362ccf2761c9531600c356317a625d9794). I can remove that commit if this is not in scope of this PR. ### Performance I'm a noob at using the tools to profile an application. Hopefully this is good enough. Perf in release mode while running `1..2000000` in Powershell during the first 4 seconds, without the last commit (686a7b362ccf2761c9531600c356317a625d9794): Before: ![before](https://user-images.githubusercontent.com/56923875/74609840-4d725b00-50ee-11ea-9e10-ea842fa1d00c.png) After: ![after](https://user-images.githubusercontent.com/56923875/74610071-30d72280-50f0-11ea-9aa8-b84fa446f8f6.png) <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed I tried to scroll with the scroll bar, with the mouse wheel. I tried to scroll while content is being outputted. I tried to reproduce the crash from #2248 without success (good).
claunia added the pull-request label 2026-01-31 09:12:16 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#25859
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?