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: Fix missing flush on console mode changes #15991

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Sep 18, 2023

Previously, all unknown escape sequences would lead to an immediate call
to VtEngine::_Flush(). This lead to problems with nushell which uses
FTCS marks that were unknown to us. Combined with the linewise redrawing
that nushell does, Terminal would get the prompt in two separate frames,
causing a slight flickering.

#14677 fixed this by suppressing the _Flush() call when unknown
sequences are encountered. Unfortunately, this triggered a bug due
to our somewhat "inconsistent" architecture in conhost:
XtermEngine::WriteTerminalW isn't just used to flush unknown sequences
but also used directly by InputBuffer::PassThroughWin32MouseRequest
to write its mouse sequence directly to the ConPTY host.
VtEngine already contains a number of specialized member functions
like RequestWin32Input() to ensure that _Flush() is called
immediately and another member could've been added to solve this issue.
This commit now adds RequestMouseMode in the same vein.

But I believe we can make the system more robust in general by using
eager flushing by default (= safe), similar to how a write() on a
TCP socket flushes by default, and instead only selectively pause and
unpause flushing with a system similar to TCP_CORK.

This seems to work fairly well, as it solves:

  • The original nushell bug
  • The new bug
  • Improves overall throughput by ~33% (due to less flushing)

In particular the last point is noteworthy, as this commit removes
the last performance bottleneck in ConPTY that isn't VtEngine.
Around ~95% of all CPU and wall time is spent in there now and any
improvements to VtEngine should yield immediately results.

Closes #15711

Validation Steps Performed

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Area-VT Virtual Terminal sequence support Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Severity-Blocking We won't ship a release like this! No-siree. labels Sep 18, 2023
@DHowett DHowett merged commit 41f7ed7 into main Sep 21, 2023
17 checks passed
@DHowett DHowett deleted the dev/lhecker/15711-setconsolemode-flush branch September 21, 2023 21:56
microsoft-github-policy-service bot pushed a commit that referenced this pull request Nov 21, 2023
This changeset fixes an issue caused by #15991 where "chunked" escape
sequences would get corrupted. The fix is to simply not flush eagerly
anymore. I tried my best to keep the input lag reduction from #15991,
but unfortunately this isn't possible for console APIs.

Closes #16079

## Validation Steps Performed
* `type ascii.com` produces soft font ASCII characters ✅
DHowett pushed a commit that referenced this pull request Dec 6, 2023
This changeset fixes an issue caused by #15991 where "chunked" escape
sequences would get corrupted. The fix is to simply not flush eagerly
anymore. I tried my best to keep the input lag reduction from #15991,
but unfortunately this isn't possible for console APIs.

Closes #16079

## Validation Steps Performed
* `type ascii.com` produces soft font ASCII characters ✅

(cherry picked from commit bdf2f6f)
Service-Card-Id: 91283205
Service-Version: 1.19
github-merge-queue bot pushed a commit that referenced this pull request Mar 28, 2024
I've found that #16079 was never properly addressed (it still randomly
occurred after even after PR #16349), which later led to the issues
described in #16769 (nushell flickering due to too many flushes).

The crux of the fix is that this brings back the `_noFlushOnEnd` flag
that was removed in PR #15991. This is then combined with a change to
the cork API: An `uncork` on `VtEngine` now only flushes if `_Flush`
got called while it was corked in the first place.

`_noFlushOnEnd` prevents us from flushing in between two "unknown"
VT sequences (like soft fonts or FTCS) which prevents them from being
corrupted. The corking prevents the remaining cases of flushing too
often. Long-term, a proper fix would be to pass through VT unmodified.

Closes #16769
DHowett pushed a commit that referenced this pull request Apr 16, 2024
I've found that #16079 was never properly addressed (it still randomly
occurred after even after PR #16349), which later led to the issues
described in #16769 (nushell flickering due to too many flushes).

The crux of the fix is that this brings back the `_noFlushOnEnd` flag
that was removed in PR #15991. This is then combined with a change to
the cork API: An `uncork` on `VtEngine` now only flushes if `_Flush`
got called while it was corked in the first place.

`_noFlushOnEnd` prevents us from flushing in between two "unknown"
VT sequences (like soft fonts or FTCS) which prevents them from being
corrupted. The corking prevents the remaining cases of flushing too
often. Long-term, a proper fix would be to pass through VT unmodified.

Closes #16769

(cherry picked from commit 1ede023)
Service-Card-Id: 91965217
Service-Version: 1.20
DHowett pushed a commit that referenced this pull request Apr 16, 2024
I've found that #16079 was never properly addressed (it still randomly
occurred after even after PR #16349), which later led to the issues
described in #16769 (nushell flickering due to too many flushes).

The crux of the fix is that this brings back the `_noFlushOnEnd` flag
that was removed in PR #15991. This is then combined with a change to
the cork API: An `uncork` on `VtEngine` now only flushes if `_Flush`
got called while it was corked in the first place.

`_noFlushOnEnd` prevents us from flushing in between two "unknown"
VT sequences (like soft fonts or FTCS) which prevents them from being
corrupted. The corking prevents the remaining cases of flushing too
often. Long-term, a proper fix would be to pass through VT unmodified.

Closes #16769

(cherry picked from commit 1ede023)
Service-Card-Id: 91965216
Service-Version: 1.19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Severity-Blocking We won't ship a release like this! No-siree. zBugBash-Consider
Projects
Status: To Consider
3 participants