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

Conpty: Additional ScrollFrame optimizations #5228

Closed
1 of 3 tasks
zadjii-msft opened this issue Apr 3, 2020 · 1 comment · Fixed by #17510
Closed
1 of 3 tasks

Conpty: Additional ScrollFrame optimizations #5228

zadjii-msft opened this issue Apr 3, 2020 · 1 comment · Fixed by #17510
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty
Milestone

Comments

@zadjii-msft
Copy link
Member

zadjii-msft commented Apr 3, 2020

In #5181, I changed XtermEngine::ScrollFrame to actually work.

Part of the trickiness with that function is that we need to make sure to emit newlines to scroll new lines into the terminal, but we also need to preserve linewrap state for lines that might have been split across paints.

  • If only the bottom line is invalid, don't manually emit a newline.
    • We need to manually emit newlines to make sure there's space at the bottom of the terminal's buffer. However, if only the bottom line is invalid, then we're going to emit a newline to get to the next line of the buffer anyways. We only need the newlines if there's other invalid regions in the buffer, because those regions will get painted (to the wrong place) before we newline to the new bottom line.
    • This includes "don't mark the last cell as invalid" for a wrapped line
  • Also discussed at this time: use a bitset to track which lines are new instead of the single _newBottomLine member. This is probably more correct for cases where multiple lines are getting scrolled in at a time.
  • conpty exhibits pathological performance on scrolling region redraw (repaints entire screen) #7019
@zadjii-msft zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Conpty For console issues specifically related to conpty Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) labels Apr 3, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Apr 3, 2020
@zadjii-msft zadjii-msft added this to the 21H1 milestone Apr 3, 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 Apr 7, 2020
@DHowett-MSFT
Copy link
Contributor

Triaged- 21H1 is correct here.

ghost pushed a commit that referenced this issue Apr 9, 2020
Now that the Terminal is doing a better job of actually marking which
lines were and were not wrapped, we're not always copying lines as
"wrapped" when they should be. We're more correctly marking lines as not
wrapped, when previously we'd leave them marked wrapped.

The real problem is here in the `ScrollFrame` method - we'd manually
newline the cursor to make the terminal's viewport shift down to a new
line. If we had to scroll the viewport for a _wrapped_ line, this would
cause the Terminal to mark that line as broken, because conpty would
emit an extra `\n` that didn't actually exist.

This more correctly implements `ScrollFrame`. Now, well move where we
"thought" the cursor was, so when we get to the next `PaintBufferLine`,
if the cursor needs to newline for the next line, it'll newline, but if
we're in the middle of a wrapped line, we'll just keep printing the
wrapped line.

A couple follow up bugs were found to be caused by the same bad logic.
See #5039 and #5161 for more details on the investigations there.

## References

* #4741 RwR, which probably made this worse
* #5122, which I branched off of 
* #1245, #357 - a pair of other conpty wrapped lines bugs
* #5228 - A followup issue for this PR

## PR Checklist
* [x] Closes #5113
* [x] Closes #5180 (by fixing DECRST 25)
* [x] Closes #5039
* [x] Closes #5161 (by ensuring we only `removeSpaces` on the actual
  bottom line)
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed

* Checked the cases from #1245, #357 to validate that they still work
* Added more and more tests for these scenarios, and then I added MORE
  tests
* The entire team played with this in selfhost builds
@zadjii-msft zadjii-msft modified the milestones: Console Backlog, Backlog Jan 4, 2022
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Aug 1, 2024
@lhecker lhecker closed this as completed in 450eec4 Aug 1, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants