WIP: Pass through VT input unmodified

[lhecker]

⚠️ This is just an idea for discussion. What do you all think about this?

❌ This cannot be merged as is, because I modified Terminal::_systemMode, but I actually need to transmit the DISABLE_NEWLINE_AUTO_RETURN flag from the conhost side properly. I don't know how to do that and only after I do, this can be merged.

This follows a different approach than the original. Instead of splitting the console API into a non-VT and VT style implementation via an interface and virtual dispatch, this one will focus on using if/else conditions. Most importantly, it hooks into WriteChars() to bypass the VtEngine renderer and send the given input directly to the ConPTY client. This ensures we preserve the VT input as-is and improves throughput ~7x.

My hope is that in the future we don't disable VtEngine, but rather enable it for all console APIs individually and flush it before returning from the console call. Then we can work towards reimplementing each such console API in terms of VT sequences if ConPTY is enabled (i.e. using if() and early returns).

This still doesn't work 100% with pwsh. It doubles each typed character and then only removes every second character when backspacing. I can't quite figure out what's going on.

Validation Steps Performed

TBD

[j4james]

I've only briefly skimmed the code, but it seems like you're passing through the VT output directly to the conpty client, but also parsing it so it's rendered into the conhost buffer. And then the legacy console APIs still operate on the conhost buffer and flush any changes via the VtEngine as they currently do.

The problem with this approach is that the conhost buffer and the conpty client's buffer will inevitably get out of sync, and then any legacy console APIs will be rendering in the wrong place and corrupting the output. So while it may be faster, it doesn't solve the problem of passing through unsupported sequences which desync the conpty client from conhost.

Also, I thought the biggest problem with passthrough was in the handling of legacy state queries, which are synchronous (e.g. things like the cursor position and palette). I'm assuming you're relying on that state being tracked in conhost, which is not much worse than the existing conpty implementation, but it doesn't address client-side state changes, or anything triggered by the passthrough which is out of sync with the conhost side.

[o-sdn-o]

Is there a possibility that in the future some kind of condrv interface will be provided to 3p terminals, so that these terminals can process Console API calls directly in their buffers without intermediaries? Would you like this for WT? This immediately solves a gazillion issues with performance and synchronization.

[zadjii-msft]

FWIW this is more like what I had originally envisioned for passthrough. IMO I only ever wanted passthrough for apps that enabled it manually - "I solemnly swear that I am up to good and won't fuck with the console APIs anymore".

Since passthrough's basically useless now, we may as well try to improve it a bit here. It can't hurt. I'm not sure it'll ever work great for apps that are mixing and matching VT and console APIs.

[o-sdn-o]

Everyone understands that there is no pure VT here. That with the current approach, Console API calls are inevitable. Strictly speaking, std::cout << "text" is always a call to WriteConsole("text"), std::cin >> var orReadFile() is always a call to ReadConsole() . From the point of view of the operating system, console applications use only the Console API and nothing else, and here, in fact, the concept of stdin/stdout is completely absent.

Since there is no VT, it makes sense to process all Console API calls on the terminal side.

Would you like to know how it would work on a live example if it were implemented? A year ago, I implemented this approach in my terminal emulator. Since then, all Console API calls are processed on the terminal side directly in its buffers. For almost a year of flight, there was not a single issue related to performance or synchronization. Nothing has changed for applications - cmd.exe, powershell.exe, pwsh.exe, wsl.exe, far.exe, mc, git, vim - all console applications run with high performance and without issues. Moreover, all Unix applications work whether they are in a Unix environment.

[o-sdn-o]

I'm not sure it'll ever work great for apps that are mixing and matching VT and console APIs.

See, the application calls WriteConsole("vt_sequence") (or std::cout << "vt_sequence" doesn't matter) - the terminal processes the WriteConsole and parses the "vt_sequence" by applying it to its buffer. The application calls SetConsoleCursorPosition(row, col) - the terminal sets the cursor, just as if it had received "\e[row;colH" via WriteConsole("vt_sequence"). This works in a very consistent way with all Console API functions. You can test this with vtm.

[lhecker]

Edit: Winner of the longest GitHub comment incoming...

@j4james Initially I wrote this PR as sort of a "is this even possible" experiment. My idea was that if VtEngine is never invoked as the result of an operation by a VT-enabled client, we could simplify it to produce output that doesn't try to be efficient or replicate the original input. Like let's say, we know that the til::bitmap in VtEngine has a very high CPU overhead. If we don't intend to reproduce the input as closely as possible anymore, we could just invalidate entire rows at a time. Or drop more elaborate checks like those in XtermEngine::_MoveCursor. (I'm not actually intending to do this. These are just hypothetical examples to showcase my idea.)
My hope was that if we did that, we could reduce VtEngine down to a point where we fully comprehend how it works. It seems like we don't right now, given that we have inexplicable bugs like #15623. 😅 (It's not like we can't fix such bugs, but I get the feeling that the async nature of VtEngine doesn't help make this a "10min task". 😅😅) I would've then hoped to be able to transition VtEngine from opt-out to opt-in and reimplement the console APIs one by one with code that translates from the console API parameters directly and synchronously to VT.

But I think I realized that this approach is a little flawed. I now get the feeling that the amount of simplification we could out of this is insufficient to have a meaningful, positive effect on our stability / bug count. Or am I just being pessimistic? I think I might just skip the idea from this PR and start implementing the direct-console-to-vt-translation code right away.

I know that this doesn't solve the desync issue that plagues ConPTY, but I wasn't intending to solve it. I'm also not particularly interested in the performance aspect. The truth is: I really don't know how to solve these while keeping the current ConPTY architecture (= it being an external process). That's why my focus was on reducing the complexity of VtEngine (and other, related parts) to improve the long-term maintainability of this project and in turn reduce the number of unexpected bugs. I wouldn't mind if we had to trade a large number of potential bugs that are unexpected with a small number of entirely expected bugs to be honest, because that'd still put us into a better place than before IMO. 😅

Due to ConPTY running in a different process it'll never reach the performance that OpenConsole has. With all my current and future performance PRs merged in, OpenConsole runs at >300MB/s whereas ConPTY with passthrough still only runs at ~50MB/s - the exact same speed as when OpenConsole still ran at ~100MB/s. It's practically entirely bottlenecked by the cross-process communication (and the redundant VT parsing).
And the desync issues of course... I don't see any way for us to use COM or similar to sync the state between WT (or whatever) and OpenConsole that doesn't result in the risk of deadlocks, hangs or lags, similar to the issues we currently have (just less bad, but still not okay). Like resizing the buffer for instance: How would one do that, short of writing a resize request into the control pipe and then waiting for ConPTY to write a custom "window size" VT sequence into the output pipe and only then we'd reflow WT's text buffer. I feel like the "rubber banding" this results in would be better than it is now, but still not be great. But things like cursor positioning would probably be even harder to do correctly, right?
The only way I can personally imagine fixing this properly is to make ConPTY simply not run out of process and that's infeasible for existing consumers of ConPTY. There's still the possibility to build a conhost.lib that offers a framework for people to implement the condrv interface themselves more easily and/or offer utility functions to translate console calls to VT. That would properly fix the performance issues and might be easy to integrate if we make the library easy to consume. That would require removing ServiceLocator however and that's... something.
In other words, preparing the current ConPTY for long term maintenance is IMO the best we can do right now, and I personally believe trimming it down wherever we can, might be the best way to do so.
Or am I missing things? Is there an easier way? Is anything above incorrect? If there is a better approach, I'd be happy to do it.

@zadjii-msft I'm not sure whether such an opt-in would meaningfully improve things over just implementing passthrough mode for all VT apps. I'm a little scared that such an opt-in would break applications that integrate 3rd party libraries and thus would end up not being heavily used. If we can build a "vt renderer" that can translate things, I'm convinced we can build regular functions that can translate them synchronously/directly as well. Such functions would also have the major benefit that we could unit test them more easily, because they'd be effectively state-less (even if they consume state, like the current buffer width, etc.).

@o-sdn-o First of all, I saw console implementation a while ago and it's absolutely fantastic. Nice work! I'm not sure if I understand what you're saying, however. What you're suggesting is effectively the same as my suggestion above to have something like a "conhost.lib" right?

There's still the possibility to build a conhost.lib that offers a framework for people to implement the condrv interface themselves more easily and/or offer utility functions to translate console calls to VT. That would properly fix the performance issues and might be easy to integrate if we make the library easy to consume.

I personally would really enjoy if we had something like that but given our extremely limited resources and us not being able to write a "clean sheet" conhost like in vtm, while also having tons of complex feature requests in our backlog, we're backlogged for a very long time. I'm also not sure how many people would integrate such a library in the first place. I'd personally be happy about it, because it would solve one of 2 performance regressions, but... it'd be hard to sell working for months on this (extracting it out of conhost, testing shipping, ...).

[j4james]

My idea was that if VtEngine is never invoked as the result of an operation by a VT-enabled client, we could simplify it to produce output that doesn't try to be efficient or replicate the original input.

@lhecker I totally agree with this. But ideally the legacy console APIs wouldn't need to rely on the VtEngine either. It would only be kept as a fallback mode for apps that depended on operations that passthrough can't support (e.g. reading from the conhost buffer).

I would've then hoped to be able to transition VtEngine from opt-out to opt-in and reimplement the console APIs one by one with code that translates from the console API parameters directly and synchronously to VT.

OK, yeah, that sounds like we're thinking along the same lines, although I would just go all in with translating the APIs from the start, which I think is what miniksa's implementation was going for. That's why I wanted to get all the VT operations in place that we'd need for this.

It's practically entirely bottlenecked by the cross-process communication (and the redundant VT parsing).

But note that the redundant VT parsing would be dropped in a pure passthrough mode. There wouldn't be a conhost buffer at all.

And the desync issues of course... I don't see any way for us to use COM or similar to sync the state between WT (or whatever) and OpenConsole that doesn't result in the risk of deadlocks, hangs or lags

Remember that this is not just about syncing between processes (i.e. conhost and WT). One of the main purposes of conpty is to support console applications over a remote connection like ssh. So this stuff has be implemented with VT sequences somehow. I suppose you could use some other protocol, like DCOM, but I don't think that makes anything easier.

My view is that maybe we just have to live with the fact that these operations are potentially going to be slow in passthrough mode. So if you're calling an API to get the cursor position, it's going to send a DSR-CPR down the line and block until it gets something back.

As a later optimisation, we could try and cache certain queries, especially if we're willing to do at least some parsing of the passed-through content. Like if we can tell that we haven't passed through a palette changing sequence, then we don't need to requery the palette every time GetConsoleScreenBufferInfoEx is called (user changes via the settings are a separate problem though).

Like resizing the buffer for instance: How would one do that, short of writing a resize request into the control pipe and then waiting for ConPTY to write a custom "window size" VT sequence into the output pipe and only then we'd reflow WT's text buffer.

Unless I'm missing something, I don't think that's a problem. With a pure passthrough mode, conhost doesn't have a buffer, so it doesn't need to know anything about the size of the client buffer, except when someone actually queries that information. Just reporting the size to conhost when it's changed in the client should be enough I think.

But things like cursor positioning would probably be even harder to do correctly, right?

Yeah, that's the biggest problem in my opinion, because I think some console apps query the cursor position non-stop, which would require a constant stream of blocking DSR-CPR requests. But maybe we just have to accept those kinds of apps are going to be a bit sluggish, or you need to run them using the non-passthrough mode.

There's still the possibility to build a conhost.lib that offers a framework for people to implement the condrv interface themselves

That's an orthogonal issue I think, and should be discussed separately. Unless I'm misunderstanding the concept, that isn't going to get legacy console apps running over ssh. That's just about optimising third-party Windows terminals running locally.

@zadjii-msft I'm not sure whether such an opt-in would meaningfully improve things over just implementing passthrough mode for all VT apps.

Yeah, if apps are required to opt-in to passthrough mode it seems kind of pointless to me. I'd expect cmd shell operations like ECHO and TYPE to work out of the box (e.g. echoing a sixel sequence, or viewing a sixel file). I can't see the cmd shell being updated to support that, and it shouldn't have to be.

[o-sdn-o]

Remember that this is not just about syncing between processes (i.e. conhost and WT). One of the main purposes of conpty is to support console applications over a remote connection like ssh. So this stuff has be implemented with VT sequences somehow. I suppose you could use some other protocol, like DCOM, but I don't think that makes anything easier.

Unless I'm misunderstanding the concept, that isn't going to get legacy console apps running over ssh. That's just about optimising third-party Windows terminals running locally.

@j4james For example, vtm does not need ConPTY at all to function remotely via SSH. Any (even legacy) console application could be launched inside a remotely launched vtm, and as a result, a pure VT stream goes via SSH to the local terminal.

[lhecker]

Thanks for your feedback! I does reduce my confidence that I'd be able to come up with an optimal system, however, because I wouldn't quite know at the moment how to implement several of the things that you mentioned. 😟 That aside, there's just one detail I don't quite get...
You wrote:

But note that the redundant VT parsing would be dropped in a pure passthrough mode. There wouldn't be a conhost buffer at all.

but then also said:

Yeah, if apps are required to opt-in to passthrough mode it seems kind of pointless to me. I'd expect cmd shell operations like ECHO and TYPE to work out of the box (e.g. echoing a sixel sequence, or viewing a sixel file). I can't see the cmd shell being updated to support that, and it shouldn't have to be.

How would ConPTY work if we don't have a buffer but simultaneously want to make it work with applications that might call console functions that require a buffer (WriteConsoleOutputAttribute for instance)?

I really like everything that you said, just for this one thing however I'd like to make a counter-argument:

That's an orthogonal issue I think, and should be discussed separately. Unless I'm misunderstanding the concept, that isn't going to get legacy console apps running over ssh. That's just about optimising third-party Windows terminals running locally.

I really don't want to push for making such a .lib just yet. I said it more as a long term vision/idea. I feel like we have many more important issues to address first (although it'd be nice if we could get rid of ServiceLocator in the meantime).
That said, I'm pretty sure it would work. On the client-side WT would use the .lib to spawn ssh.exe and ensure that its output is VT (which it already should be, so barely any conversion will be done). On the server-side, sshd.exe will also have integrated the .lib to spawn whatever console application you wanted and turn its output into VT to send it back to the client. It should work exactly like ConPTY but in-process. Such a .lib would have the advantage that we could provide the entire spectrum of APIs from "here's the individual components to create a condrv client, the code to turn console API A,B,C,D,E,... into VT output as long as you give me your text-buffer's information, here's how to process these flags into those flags, etc." all the way to "call this one function to create a ready-to-use in-process ConPTY with our default TextBuffer, our default console API implementation, etc.". In fact, we could could use this to build conhost itself. We could probably even use it to implement the Win32 CreatePseudoConsole API with it, but I'm afraid that will be difficult to convince people for it.
And it would help make local apps run as good as they used to. While calling it a performance optimization is 100% fair, I feel like saying that it fixes a regression (and thus isn't an optimization) makes it a much more convincing argument to spend time on this. It used to work "this well" and now it's "less well" after all.
But I like the saying "make it work, make it right, make it fast" and I feel like we're squarely at the 2nd one right now. So, all the above is just me dreaming about what we could do once we're done with that. 😅

Edit: I just realized that me saying that sounds kind of silly, given my recent barrage of performance PRs. But that's just me being self-indulgent. Like eating too much chocolate. 😄

@o-sdn-o Your feedback and help is absolutely invaluable, but I would like to at least just once point out that your implementation isn't quite complete, purely from reading your source code. Maybe it works well in practice, but I'm afraid it might be at least somewhat more complex once you implement all the remaining terrible legacy nuances that ConPTY handles. (I mean it probably will never be as complex as our current code, since yours is a clean-sheet implementation, but... Well, you probably know what I mean...)

[o-sdn-o]

Maybe it works well in practice, but I'm afraid it might be at least somewhat more complex once you implement all the remaining terrible legacy nuances that ConPTY handles.

@lhecker If something is not implemented there, then this is most likely done intentionally for security reasons, for example, as is the case with the DOSKEY functionality.

And so, of course, you are right, there are such cases. But this is a question of the need for such functionality. If there is a need for a certain legacy function, then it is immediately directly implemented.

[o-sdn-o]

There's still the possibility to build a conhost.lib that offers a framework for people to implement the condrv interface themselves more easily and/or offer utility functions to translate console calls to VT. That would properly fix the performance issues and might be easy to integrate if we make the library easy to consume.

@lhecker In general, the complexity of handling Console API calls is added by the fact that some of them require an immediate response with some result.

[j4james]

How would ConPTY work if we don't have a buffer but simultaneously want to make it work with applications that might call console functions that require a buffer (WriteConsoleOutputAttribute for instance)?

@lhecker Well I was kind of hoping that there wouldn't be any of significance. I haven't actually gone through the whole list, so I might be wrong about that, but I know at least WriteConsoleOutputAttribute can be translated to DECCARA. And while I know we can't read from the buffer, I was hoping we could survive without that. And technically we could even hack some form of support for buffer reading with DECRQCRA if we really wanted to.

Then in the rare case (hopefully) that you have an app that doesn't work well with passthrough, you can open a profile with the non-passthrough version of conpty, and run the app from there. Worst case we just leave the current conpty implementation as the default, and you switch to the passthrough profile only when needed, or only enable it by default for WSL shells.

I really don't want to push for making such a .lib just yet. I said it more as a long term vision/idea.

But if doesn't have anything to do with conpty passthrough, then I don't get why we're discussing it here. Like do we need a condrv lib in order to support conpty passthrough, or do we need conpty passthrough to support a condrv lib? If the answer is no in both cases, then it's just sidetracking us. We should open a separate issue for implementing a condrv lib and anyone interested in that can discuss the subject there.

[lhecker]

Hmm I see what you mean... I'll stop using "passthrough" as a tag/phrase for my work in this area then.

If the passthrough mode, as originally described, requires explicit opt-in then many won't use it. Explicit opt-out is unfeasible unless it works with all console applications out of the box. If it were to work with all console applications out of the box, I'd be happy to pursue it right away, because this would allow me to replace the existing VT output code 1:1 with new VT output code.
But that seems somewhat unlikely, doesn't it? So right now, at least personally speaking, I don't wish to pursue adding an entirely separate path for VT applications. We already struggle to maintain the code base as is, falling behind our numerous feature requests and bug filings, and I feel like it wouldn't help if we add another complex feature on top.
As such I'd rather first make the existing ConPTY be the best ConPTY it can be, before further refining it with the passthrough mode. Maybe I'll call this work "direct translation" or something. That's what I intended with this PR. Making the existing system better without the need for a passthrough-opt-in.

We should open a separate issue for implementing a condrv lib and anyone interested in that can discuss the subject there.

I only wrote that in response to you mentioning the desync issue. It's the only robust solution to that problem that I can personally think of right now and it doesn't have anything to do with passing through input unmodified. I didn't mean to detract attention from the PR's contents. I'm sorry.

[j4james]

Explicit opt-out is unfeasible unless it works with all console applications out of the box.

I'll just note that Windows Terminal already doesn't work with all console applications out of the box.

Maybe I'll call this work "direct translation" or something. That's what I intended with this PR. Making the existing system better without the need for a passthrough-opt-in.

I should probably reserve judgement until it's out of draft, but my initial impression was that this PR would make things worse. But if you can get it working somehow that would be brilliant, because it does seem a lot simpler than a full passthrough.

[DHowett]

There's still the possibility to build a conhost.lib that offers a framework for people to implement the condrv interface themselves

Just driving by to say that I am profoundly averse to this solution, because I do not believe we should expose random applications to the implementation details of condrv or console API servicing. It brings us further away from the goal of converging on VT as a standard rather than closer to it.

[j4james]

@lhecker In case it helps, these are some of the areas I thought might be problematic when I was investigating this before:

1. Who responds to queries? Like if an apps sends a DA request, or a DSR-CPR, does conhost respond to that, or do you let the conpty client handle them?
2. How do you deal with operations that the conpty client supports but that conhost doesn't? Sixel would be the most obvious one.
3. How do you deal with operations that conhost supports, but that the conpty client doesn't? Macros and horizontal margins are likely candidates here.
4. How would you get soft fonts to work in a way that won't be destroyed by a VtEngine repaint?
5. How do you deal with state that the VtEngine expects to be reset (consider things like margins, wrapping, and origin mode) while also not messing with the conpty client's expected state?

Just to be clear, I'm not expecting you to answer these question - I don't want to waste your time responding to me when you could be getting on with the code. I just want to make sure there aren't a lot of edge cases that you haven't considered.

[github-actions]

@check-spelling-bot Report

🔴 Please review

See the 📂 files view or the 📜action log for details.

Unrecognized words (1)

atvis

Previously acknowledged words that are now absent

vtapi :arrow_right:

Some files were automatically ignored

These sample patterns would exclude them:

^\Qsrc/host/PassthroughState.cpp\E$

You should consider adding them to:

.github/actions/spelling/excludes.txt

File matching is via Perl regular expressions.

To check these files, more of their words need to be in the dictionary than not. You can use patterns.txt to exclude portions, add items to the dictionary (e.g. by adding them to allow.txt), or fix typos.

To accept ✔️ these unrecognized words as correct and remove the previously acknowledged and now absent words, run the following commands

... in a clone of the git@github.com:microsoft/terminal.git repository
on the dev/lhecker/passthrough branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.21/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/6747991815/attempts/1'

Pattern suggestions ✂️ (1)

You could add these patterns to .github/actions/spelling/patterns/0052725198dbbcef0ed7a07eb00379d0d7156fd2.txt:

# Automatically suggested patterns
# hit-count: 1 file-count: 1
# tput arguments -- https://man7.org/linux/man-pages/man5/terminfo.5.html -- technically they can be more than 5 chars long...
\btput\s+(?:(?:-[SV]|-T\s*\w+)\s+)*\w{3,5}\b

Warnings (2)

See the 📂 files view or the 📜action log for details.

ℹ️ Warnings	Count
ℹ️ binary-file	1
ℹ️ candidate-pattern	1

See ℹ️ Event descriptions for more information.

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. If it doesn't work for you, you can manually add (one word per line) / remove items to expect.txt and the excludes.txt files.

If the listed items are:

• ... misspelled, then please correct them instead of using the command.
• ... names, please add them to .github/actions/spelling/allow/names.txt.
• ... APIs, you can add them to a file in .github/actions/spelling/allow/.
• ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
• ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

If the flagged items are 🤯 false positives

If items relate to a ...

• binary file (or some other file you wouldn't want to check at all).

  Please add a file path to the excludes.txt file matching the containing file.

  File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

  ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

• well-formed pattern.

  If you can write a pattern that would match it,
  try adding it to the patterns.txt file.

  Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

  Note that patterns can't match multiline strings.