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

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

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

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/4608
Author: @beviu
Created: 2/15/2020
Status: ✅ Merged
Merged: 6/12/2020
Merged by: @DHowett

Base: master ← Head: throttle-scrollbar-updates

📝 Commits (10+)

📊 Changes

12 files changed (+453 additions, -250 deletions)

View changed files

📝 .github/actions/spell-check/dictionary/apis.txt (+1 -0)
📝 src/cascadia/TerminalControl/TermControl.cpp (+51 -52)
📝 src/cascadia/TerminalControl/TermControl.h (+11 -4)
📝 src/cascadia/TerminalControl/TerminalControl.vcxproj (+2 -0)
📝 src/cascadia/TerminalControl/TerminalControl.vcxproj.filters (+2 -0)
➕ src/cascadia/TerminalControl/ThreadSafeOptional.h (+54 -0)
➕ src/cascadia/TerminalControl/ThrottledFunc.h (+97 -0)
📝 src/cascadia/TerminalCore/Terminal.cpp (+15 -14)
📝 src/cascadia/TerminalCore/Terminal.hpp (+2 -2)
➕ src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp (+217 -0)
➖ src/cascadia/UnitTests_TerminalCore/TerminalAndRendererTests.cpp (+0 -177)
📝 src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj (+1 -1)

📄 Description

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).

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/microsoft/terminal/pull/4608 **Author:** [@beviu](https://github.com/beviu) **Created:** 2/15/2020 **Status:** ✅ Merged **Merged:** 6/12/2020 **Merged by:** [@DHowett](https://github.com/DHowett) **Base:** `master` ← **Head:** `throttle-scrollbar-updates` --- ### 📝 Commits (10+) - [`bda5acc`](https://github.com/microsoft/terminal/commit/bda5acc79d3eaf5d0973c12cd6cb0c7be15abf60) Re-enable "Throttle scroll position update" - [`1508ff1`](https://github.com/microsoft/terminal/commit/1508ff1895f94373f22b609a4ee5793f30c47dee) Update scroll test to also check for `TriggerScroll` notification - [`5130184`](https://github.com/microsoft/terminal/commit/5130184ad9ad5b719e7c26d31ddc85b81bd0c32f) Refactor `Terminal::_AdjustCursorPosition` - [`2f06764`](https://github.com/microsoft/terminal/commit/2f06764edd24b666cc562bcce42b3fcb39a61896) Formatting - [`c6e72f8`](https://github.com/microsoft/terminal/commit/c6e72f8d6e45a6c7f5258bc5dba6c5744a559d49) Merge branch 'master' of https://github.com/microsoft/terminal into throttle-scrollbar-updates - [`d3b7748`](https://github.com/microsoft/terminal/commit/d3b7748ce47e9672f7757d60438685c60bea0169) Refactor - [`e1ffb92`](https://github.com/microsoft/terminal/commit/e1ffb923276f932fa74f41f174c07cdff4ff3d2d) format - [`7c106b9`](https://github.com/microsoft/terminal/commit/7c106b92204c595582635dd10088d4ec9ac05f8d) spell check - [`13b37d5`](https://github.com/microsoft/terminal/commit/13b37d51272e23ebf47e9f00bc641cbca022cf33) update doc in ThrottledFunc.h - [`b22af56`](https://github.com/microsoft/terminal/commit/b22af5681fe3a9a69f12dfa1561a93cc865d101b) don't block ### 📊 Changes **12 files changed** (+453 additions, -250 deletions) <details> <summary>View changed files</summary> 📝 `.github/actions/spell-check/dictionary/apis.txt` (+1 -0) 📝 `src/cascadia/TerminalControl/TermControl.cpp` (+51 -52) 📝 `src/cascadia/TerminalControl/TermControl.h` (+11 -4) 📝 `src/cascadia/TerminalControl/TerminalControl.vcxproj` (+2 -0) 📝 `src/cascadia/TerminalControl/TerminalControl.vcxproj.filters` (+2 -0) ➕ `src/cascadia/TerminalControl/ThreadSafeOptional.h` (+54 -0) ➕ `src/cascadia/TerminalControl/ThrottledFunc.h` (+97 -0) 📝 `src/cascadia/TerminalCore/Terminal.cpp` (+15 -14) 📝 `src/cascadia/TerminalCore/Terminal.hpp` (+2 -2) ➕ `src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp` (+217 -0) ➖ `src/cascadia/UnitTests_TerminalCore/TerminalAndRendererTests.cpp` (+0 -177) 📝 `src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj` (+1 -1) </details> ### 📄 Description <!-- 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). --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
claunia added the pull-request label 2026-01-31 09:12:15 +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#25854
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?