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.

Sign up for GitHub

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

Jump to bottom

TextBuffer::Reflow performs poorly for large buffers #4968

Closed
Tracked by #8000
Jyrka98 opened this issue Mar 17, 2020 · 1 comment · Fixed by #15701
Closed
Tracked by #8000

TextBuffer::Reflow performs poorly for large buffers #4968

Jyrka98 opened this issue Mar 17, 2020 · 1 comment · Fixed by #15701
Labels
Area-Performance Performance-related issue In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Milestone
Backlog

Comments

@Jyrka98
Copy link

Jyrka98 commented Mar 17, 2020 •
edited by DHowett-MSFT
Loading

(edited by @DHowett-MSFT)
Since we took a bigger bet on ResizeWithReflow with Terminal, we should look into its performance characteristics. Related: #2089.
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Mar 17, 2020
@DHowett-MSFT
Copy link
Contributor

Unfortunately, the terminal resized smoothly because it was broken and instead of reflowing the contents of the screen it was deleting them. For now, this is an acceptable performance hit. I'm going to make this the parent issue for "Reflow performs poorly."

@DHowett-MSFT DHowett-MSFT changed the title Worse resizing performance with version 0.10.761.0 TextBuffer::Reflow performs poorly for large buffers Mar 17, 2020
@DHowett-MSFT DHowett-MSFT added Area-Performance Performance-related issue Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Mar 17, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Mar 17, 2020
@DHowett-MSFT DHowett-MSFT added this to the Console Backlog milestone Mar 17, 2020
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Mar 17, 2020
@zadjii-msft zadjii-msft modified the milestones: Console Backlog, 22H2 Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H2, Backlog Jul 5, 2023
@zadjii-msft zadjii-msft mentioned this issue Jun 30, 2023
Closed
15 tasks
@lhecker lhecker mentioned this issue Aug 15, 2023
Merged
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Aug 15, 2023
DHowett pushed a commit that referenced this issue Sep 26, 2023
@lhecker
Reimplement TextBuffer::Reflow (#15701)
7474839 
Subjectively speaking, this commit makes 3 improvements:
* Most importantly, it now would work with arbitrary Unicode text.
  (No more `IsGlyphFullWidth` or DBCS handling during reflow.)
* Due to the simpler implementation it hopefully makes review of
  future changes and maintenance simpler. (~3x less LOC.)
* It improves perf. by 1-2 orders of magnitude.
  (At 120x9001 with a full buffer I get 60ms -> 2ms.)

Unfortunately, I'm not confident that the new code replicates the old
code exactly, because I failed to understand it. During development
I simply tried to match its behavior with what I think reflow should do.

Closes #797
Closes #3088
Closes #4968
Closes #6546
Closes #6901
Closes #15964
Closes MSFT:19446208

Related to #5800 and #8000

## Validation Steps Performed
* Unit tests ✅
* Feature tests ✅
* Reflow with a scrollback ✅
* Reflowing the cursor cell causes a forced line-wrap ✅
  (Even at the end of the buffer. ✅)
* `color 8f` and reflowing retains the background color ✅
* Enter alt buffer, Resize window, Exit alt buffer ✅
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Sep 26, 2023
DHowett pushed a commit that referenced this issue Sep 26, 2023
@lhecker @DHowett
Reimplement TextBuffer::Reflow (#15701)
aef2dbd 
Subjectively speaking, this commit makes 3 improvements:
* Most importantly, it now would work with arbitrary Unicode text.
  (No more `IsGlyphFullWidth` or DBCS handling during reflow.)
* Due to the simpler implementation it hopefully makes review of
  future changes and maintenance simpler. (~3x less LOC.)
* It improves perf. by 1-2 orders of magnitude.
  (At 120x9001 with a full buffer I get 60ms -> 2ms.)

Unfortunately, I'm not confident that the new code replicates the old
code exactly, because I failed to understand it. During development
I simply tried to match its behavior with what I think reflow should do.

Closes #797
Closes #3088
Closes #4968
Closes #6546
Closes #6901
Closes #15964
Closes MSFT:19446208

Related to #5800 and #8000

## Validation Steps Performed
* Unit tests ✅
* Feature tests ✅
* Reflow with a scrollback ✅
* Reflowing the cursor cell causes a forced line-wrap ✅
  (Even at the end of the buffer. ✅)
* `color 8f` and reflowing retains the background color ✅
* Enter alt buffer, Resize window, Exit alt buffer ✅

(cherry picked from commit 7474839)
Service-Card-Id: 90642727
Service-Version: 1.19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Performance Performance-related issue In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Reimplement TextBuffer::Reflow microsoft/terminal
3 participants
@Jyrka98 @DHowett-MSFT @zadjii-msft