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

[FHL] Make VTApiRoutines, which does VT translation for output #11264

Merged
34 commits merged into from
Mar 30, 2022
Merged

Conversation

miniksa
Copy link
Member

@miniksa miniksa commented Sep 17, 2021

Make a VTApiRoutines servicer that does minimal translations instead of
environmental simulation for some output methods. Remaining methods are
backed on the existing console host infrastructure (primarily input
related methods).

PR Checklist

  • I work here
  • It's Fix-Hack-Learn quality so it's behind a feature gate so we
    can keep refining it. But it's a start!

To turn this on, you will have to be in the Dev or Preview rings
(feature staged). Then add experimental.connection.passthroughMode: true to a profile and on the next launch, the flags will propagate down
through the ConptyConnection into the underlying Openconsole.exe
startup and tell it to use the passthrough mode instead of the full
simulation mode.

Validation Steps Performed

  • Played with it manually in CMD.exe, it seems to work mostly.
  • Played with it manually in Ubuntu WSL, it seems to work.
  • Played with it manually in Powershell and it's mostly sad. It'll get
    there.

Starts #1173

@github-actions

This comment has been minimized.

@miniksa miniksa changed the title [DRAFT] Make a VTApiRoutines servicer that does minimal translations instead of environmental simulation for some output methods [FHL] Make a VTApiRoutines servicer that does minimal translations instead of environmental simulation for some output methods Sep 17, 2021
@miniksa miniksa marked this pull request as ready for review January 12, 2022 00:41
@miniksa
Copy link
Member Author

miniksa commented Jan 12, 2022

Ugh I forgot to add a UI toggle.

@miniksa
Copy link
Member Author

miniksa commented Jan 13, 2022

Ugh I ruined all the tests...

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow okay I got through it all - honestly wasn't that hard to read for 65 files. Pleasantly refreshing to do a conhost code review.

This is a soft block. There's a LOT of TODOs left in here. Most seem like they're fine for follow-up work. I personally wouldn't bother with separate threads for each of them. I'd take a reserved thread (maybe #10001?), and add each of these lines as a check box in there. Things like the Title work for example - doesn't need an issue, just a note of future work to improve this.

It's all experimental anyways for now, so I'm 🤏 close to signing off.

src/host/lib/hostlib.vcxproj Outdated Show resolved Hide resolved
src/host/ut_lib/host.unittest.vcxproj Outdated Show resolved Hide resolved
src/host/srvinit.cpp Show resolved Hide resolved
src/host/VtIo.cpp Outdated Show resolved Hide resolved
src/host/VtIo.cpp Outdated Show resolved Hide resolved
src/host/VtApiRoutines.cpp Show resolved Hide resolved
src/host/VtApiRoutines.cpp Show resolved Hide resolved
src/host/VtApiRoutines.cpp Show resolved Hide resolved
src/renderer/vt/state.cpp Outdated Show resolved Hide resolved
src/renderer/vt/VtSequences.cpp Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 13, 2022
@zadjii-msft zadjii-msft added Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conpty For console issues specifically related to conpty Product-Terminal The new Windows Terminal. labels Jan 13, 2022
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 13, 2022
@miniksa
Copy link
Member Author

miniksa commented Jan 27, 2022

@zadjii-msft can I get you to pass this one more time?

@zadjii-msft
Copy link
Member

sure, once I fix the honks

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know what, let's just stealth yolo this into 1.13 and see what happens

src/host/VtApiRoutines.cpp Outdated Show resolved Hide resolved
@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Jan 28, 2022
Comment on lines 80 to 81
auto pfnWriteInput = std::bind(&ControlCore::_sendInputToConnection, this, std::placeholders::_1);
_terminal->SetWriteInputCallback(pfnWriteInput);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::bind is basically deprecated. I'd suggest using lambdas if you can.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hear you, but the whole remainder of this file is already doing it this way, so I'd rather stick to the pattern if that's OK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just don't totally get why this had to change so drastically ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I roll my eyes at both of you. Fine. I'll convert it back.

@miniksa
Copy link
Member Author

miniksa commented Feb 23, 2022

image

Fixed this too.

EDIT: Proof:
image

Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeet

@miniksa
Copy link
Member Author

miniksa commented Feb 23, 2022

@msftbot merge when @DHowett signs off.

<!-- You apparently are expected to include all the maxversiontested
entries separately. They are treated like an array. This one turns on Segoe
UI Variable, GH #12452 -->
<maxversiontested Id="10.0.22000.0"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may conflict on this file

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

58/67

@@ -1051,6 +1051,10 @@
<value>Enable experimental text rendering engine</value>
<comment>An option to enable an experimental text rendering engine</comment>
</data>
<data name="Profile_VtPassthrough.Header" xml:space="preserve">
<value>Enable experimental virtual terminal passthrough</value>
<comment>An option to enable experimental virtual terminal passthrough connectivity option with the underlying ConPTY</comment>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment could use some work -- it is supposed to be read by a human translator to help them understand what they are translating

Comment on lines 80 to 81
auto pfnWriteInput = std::bind(&ControlCore::_sendInputToConnection, this, std::placeholders::_1);
_terminal->SetWriteInputCallback(pfnWriteInput);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just don't totally get why this had to change so drastically ;)

{
if (_passthroughMode)
{
WI_SetFlag(flags, PSEUDOCONSOLE_PASSTHROUGH_MODE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not compile in Release. You are using a preprocessor define to control whether the constant is visible, but a constexpr if (which must compile successfully) wherein you are using the constant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, you're gating the presence of the member using the preprocessor as well.

Try a build from the commandline with /p:WindowsTerminalBranding=Release

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say, keep the member and the constant defined and only gate whether the code actually touches them based on if constexpr

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 24, 2022
@lhecker
Copy link
Member

lhecker commented Feb 24, 2022

I've benchmarked this change and found no regressions.
However I also found that setting the new option has only a small positive impact on throughput of 7.2MB/s -> 7.9MB/s (+9%).
OpenConsole's in-proc architecture still has a significant advantage here with 21MB/s.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 25, 2022
@miniksa
Copy link
Member Author

miniksa commented Feb 28, 2022

OK that's fine. I'm not expecting this change alone to make a massive difference but to rather remove a massive part of the system for further optimization purposes.

@lhecker
Copy link
Member

lhecker commented Feb 28, 2022

OK that's fine. I'm not expecting this change alone to make a massive difference but to rather remove a massive part of the system for further optimization purposes.

The impact is surely greater for VT-heavy applications. But I couldn't test that yet, since the output is broken for my benchmark applications.

src/host/VtIo.cpp Outdated Show resolved Hide resolved
Let the compiler do dead code elimination or whatever
@DHowett DHowett changed the title [FHL] Make a VTApiRoutines servicer that does minimal translations instead of environmental simulation for some output methods [FHL] Make a VTApiRoutines servicer that does VT translation for output Mar 30, 2022
@DHowett DHowett changed the title [FHL] Make a VTApiRoutines servicer that does VT translation for output [FHL] Make, VTApiRoutines, which does VT translation for output Mar 30, 2022
@DHowett DHowett changed the title [FHL] Make, VTApiRoutines, which does VT translation for output [FHL] Make VTApiRoutines, which does VT translation for output Mar 30, 2022
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 30, 2022
@ghost
Copy link

ghost commented Mar 30, 2022

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 2c2f4f9 into main Mar 30, 2022
@ghost ghost deleted the dev/miniksa/vtapi branch March 30, 2022 23:22
ghost pushed a commit that referenced this pull request May 17, 2022
## Summary of the Pull Request

When the conpty passthrough mode is enabled, it often needs to send `DSR-CPR` queries (cursor position reports) to the client terminal to obtain the current cursor position. However, the code that originally handled the responses to these queries got broken by the refactoring of the `ConGetSet` API. This PR is an attempt to correct that regression.

## References

The conpty passthrough mode was introduced in PR #11264.
The refactoring that broke the cursor position handling was in PR #12703.

## PR Checklist
* [x] Closes #13106
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema 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

Prior to the `ConGetSet` refactoring, the code that handled `DSR-CPR` responses (`InteractDispatch::MoveCursor`) would pass the cursor position to `ConGetSet::SetCursorPosition`, which in turn would forward it to the `SetConsoleCursorPositionImpl` API, and from there to the `VtIo` class.

After the refactor, all of those intermediate steps were removed - the cursor was simply updated directly in `InteractDispatch::MoveCursor`, and the `VtIo` call was moved from `SetConsoleCursorPositionImpl` to `InteractDispatch` (since that was the only place it was actually required).

However, when the conpty passthrough mode was introduced - which happened in parallel - it relied on the `SetConsoleCursorPositionImpl` API being called from `InteractDispatch` in order to handle its own `DSR-CPR` responses, and that's why things stopped working when the two PRs merged.

So what I've done now is made `InteractDispatch::MoveCursor` method call `SetConsoleCursorPositionImpl` again (although without the intermediate `ConGetSet` overhead), and moved the `VtIo::SetCursorPosition` call back into `SetConsoleCursorPositionImpl`.

This is not ideal, and there are still a bunch of problems with the `DSR-CPR` handling in passthrough mode, but it's at least as good as it was before.

## Validation Steps Performed

I've just manually tested various shells with passthrough mode enabled, and confirmed that they're working better now. There are still issues, but nothing that wasn't already a problem in the initial implementation, at least as far as I can tell.
carlos-zamora pushed a commit that referenced this pull request May 17, 2022
## Summary of the Pull Request

When the conpty passthrough mode is enabled, it often needs to send `DSR-CPR` queries (cursor position reports) to the client terminal to obtain the current cursor position. However, the code that originally handled the responses to these queries got broken by the refactoring of the `ConGetSet` API. This PR is an attempt to correct that regression.

## References

The conpty passthrough mode was introduced in PR #11264.
The refactoring that broke the cursor position handling was in PR #12703.

## PR Checklist
* [x] Closes #13106
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema 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

Prior to the `ConGetSet` refactoring, the code that handled `DSR-CPR` responses (`InteractDispatch::MoveCursor`) would pass the cursor position to `ConGetSet::SetCursorPosition`, which in turn would forward it to the `SetConsoleCursorPositionImpl` API, and from there to the `VtIo` class.

After the refactor, all of those intermediate steps were removed - the cursor was simply updated directly in `InteractDispatch::MoveCursor`, and the `VtIo` call was moved from `SetConsoleCursorPositionImpl` to `InteractDispatch` (since that was the only place it was actually required).

However, when the conpty passthrough mode was introduced - which happened in parallel - it relied on the `SetConsoleCursorPositionImpl` API being called from `InteractDispatch` in order to handle its own `DSR-CPR` responses, and that's why things stopped working when the two PRs merged.

So what I've done now is made `InteractDispatch::MoveCursor` method call `SetConsoleCursorPositionImpl` again (although without the intermediate `ConGetSet` overhead), and moved the `VtIo::SetCursorPosition` call back into `SetConsoleCursorPositionImpl`.

This is not ideal, and there are still a bunch of problems with the `DSR-CPR` handling in passthrough mode, but it's at least as good as it was before.

## Validation Steps Performed

I've just manually tested various shells with passthrough mode enabled, and confirmed that they're working better now. There are still issues, but nothing that wasn't already a problem in the initial implementation, at least as far as I can tell.

(cherry picked from commit e1086de)
Service-Card-Id: 82005205
Service-Version: 1.14
@ghost
Copy link

ghost commented May 24, 2022

🎉Windows Terminal Preview v1.14.143 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
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. AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Second It's a PR that needs another sign-off Product-Conpty For console issues specifically related to conpty Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants