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

Add support for Windows ConPTY API #1762

Merged
merged 5 commits into from Dec 28, 2018

Conversation

Projects
None yet
8 participants
@davidhewitt
Copy link
Contributor

davidhewitt commented Nov 9, 2018

The support is added behind a config flag enable_experimental_conpty_backend, defaulted to false. When set to true then before the WinPty initialisation alacritty will try to spawn a Pseudoconsole process, and use that instead.

I can confirm #1662 is fixed by this (and possibly others, I haven't searched too extensively). However there's a couple of pain points which is why I wanted to mark this experimental:

  • The ConPTY API only support synchronous pipes (rather than the asynchronous Evented "NamedPipe" from mio_named_pipes). So windows.rs has to change to dispatch over the two possible pipe types, and I added a local crate mio_anonymous_pipes to make these things Evented (using worker thread + intermediate buffer). Performance on those can likely be improved, tbh I'm hoping the problem goes away and we can move everything to NamedPipe if Microsoft lands Microsoft/console#262 soon.

  • Sometimes the last column has characters from other lines "ghosting" after scrolls or other writes to the buffer. I haven't used Alacritty in WinPty mode much so I'm not sure if the problem is also present there. I suspect it's an issue where the ConPTY api is treating the cursor position as having moved to the next line already when alacritty thinks it's in the last column, but I want to investigate this further after we have some other user experience.

Fixes #1661.
Fixes #1926.

@chrisduerr

This comment has been minimized.

Copy link
Collaborator

chrisduerr commented Nov 9, 2018

Sometimes the last column has characters from other lines "ghosting" after scrolls or other writes to the buffer. I haven't used Alacritty in WinPty mode much so I'm not sure if the problem is also present there. I suspect it's an issue where the ConPTY api is treating the cursor position as having moved to the next line already when alacritty thinks it's in the last column, but I want to investigate this further after we have some other user experience.

This could be related to #1317.

@jwilm jwilm requested a review from zacps Nov 9, 2018

@jwilm

This comment has been minimized.

Copy link
Owner

jwilm commented Nov 9, 2018

Wow, thanks for the PR! I'm excited to see ConPTY support so quickly. Will leave this to @zacps to review.

🎉

@zacps
Copy link
Collaborator

zacps left a comment

Thanks for the work! I've taken an initial look at the changes and have some (mostly) minor comments. There's also a couple of bigger things I'm going to follow up on at some point (like performance).

Testing will have to wait for a bit until I'm ready to update to 1809.

Show resolved Hide resolved Cargo.toml Outdated
Show resolved Hide resolved src/main.rs
Show resolved Hide resolved src/tty/windows.rs Outdated
Show resolved Hide resolved src/tty/windows.rs Outdated
Show resolved Hide resolved src/tty/windows.rs Outdated
Show resolved Hide resolved src/tty/windows.rs Outdated
Show resolved Hide resolved src/tty/windows.rs Outdated
Show resolved Hide resolved src/tty/windows.rs Outdated
Show resolved Hide resolved src/tty/windows.rs Outdated
Show resolved Hide resolved src/tty/windows.rs Outdated
@davidhewitt

This comment has been minimized.

Copy link
Contributor

davidhewitt commented Nov 11, 2018

Thanks for the great feedback! Will take a second pass this week ahead.

This could be related to #1317.

Interesting, I'm not very knowledgable about font glyphs, might try to grok that after this PR's done.

@davidhewitt

This comment has been minimized.

Copy link
Contributor

davidhewitt commented Nov 17, 2018

@zacps the comments you've made should have been addressed. Have done the various tidy ups and switched back to static dispatch (using an enum to switch between the pipe types, though with any luck microsoft's overlapped i/o implementation will come along and we can use NamedPipe only).

How do you guys want to proceed with this? It's still very much experimental - I've noticed some odd cursor flickering to 0,0 which I think might be related to Microsoft/console#235. And I suspect if microsoft adds overlapped i/o support (which I really think we should use rather than mio-anonymous-pipes, as we'll have much cleaner code and likely better performance) then it'll come in the April 2019 update or later, so users on October 2018 update might not be able to use the Conpty API once we switch to that.

My personal view would be to merge but keep it feature gated in the config until we're comfortable that we can ensure stability & performance going forward.

@parkovski

This comment has been minimized.

Copy link

parkovski commented Nov 18, 2018

@davidhewitt Curious to try this out on the latest insider build - I used to get lots of cursor flickering using PowerShell under tmux, but don't anymore, so maybe the fix for that issue has made it out to insiders already?

@davidhewitt

This comment has been minimized.

Copy link
Contributor

davidhewitt commented Nov 18, 2018

@parkovski quite possibly - I'm on the slow ring for my only pc so it'll hopefully make it to me before long!

Show resolved Hide resolved src/tty/windows/conpty.rs Outdated
@zacps

zacps approved these changes Nov 24, 2018

@zacps

This comment has been minimized.

Copy link
Collaborator

zacps commented Nov 24, 2018

Thanks for those changes! One minor comment about a comment, but other than that looking good.

I've switched to the targeted channel but I still haven't gotten 1809. After that I can start testing.

@chrisduerr
Copy link
Collaborator

chrisduerr left a comment

There's still a few remaining issues that should be resolved. I don't think there's any rush here, so I'd prefer it if the implementation would be as clean as possible.

Show resolved Hide resolved Cargo.toml Outdated
Show resolved Hide resolved alacritty_windows.yml Outdated
Show resolved Hide resolved mio-anonymous-pipes/README.md Outdated
Show resolved Hide resolved mio-anonymous-pipes/src/lib.rs Outdated
Show resolved Hide resolved mio-anonymous-pipes/src/lib.rs Outdated
Show resolved Hide resolved src/config.rs
Show resolved Hide resolved src/tty/windows/conpty.rs Outdated
Show resolved Hide resolved mio-anonymous-pipes/src/lib.rs Outdated
Show resolved Hide resolved mio-anonymous-pipes/src/lib.rs Outdated
@davidhewitt

This comment has been minimized.

Copy link
Contributor

davidhewitt commented Nov 25, 2018

Agreed there is no rush, happy to land a more complete implementation first if that is desired.

I just updated to the windows insiders fast ring and can confirm the cursor flickering to (0,0) is resolved, which is much nicer. I'm unsure if this fix will make it to non-insiders before next year's April 2019 update.

Regarding benchmarks - are there benchmarks already in alacritty I can use to compare against WinPty? Have I understood correctly that #1653 would introduce benchmarking which I could then use here?

@damnskippy

This comment has been minimized.

Copy link

damnskippy commented Nov 25, 2018

Just curious, will this PR help to get Ctrl+Space keybinding work? (#1703).

@jwilm

This comment has been minimized.

Copy link
Owner

jwilm commented Nov 27, 2018

Given that this is behind an opt-in feature flag, I think we could safely pull this in without too much delay.

@zacps

This comment has been minimized.

Copy link
Collaborator

zacps commented Nov 27, 2018

I would really rather have some chance to test this before it gets merged. Due to the delays in the 1809 rollout most people won't have access to it yet anyway.

@chrisduerr

This comment has been minimized.

Copy link
Collaborator

chrisduerr commented Nov 27, 2018

I also think making sure that code quality is decent would be a great idea. Because chances are it's never going to be improved after it's been pulled in.

@siler
Copy link

siler left a comment

I don't see a CONTRIBUTING.md but how do we feel about running new files through rust_fmt? I checked a couple others and they are pretty close but the src/tty/windows folder could use some (at least for use organization).

Show resolved Hide resolved src/tty/windows/conpty.rs Outdated
@chrisduerr

This comment has been minimized.

Copy link
Collaborator

chrisduerr commented Nov 30, 2018

@siler Using rustfmt has been discussed before, however it doesn't make much sense to do it until at least some of the open PRs are merged.

@jwilm

This comment has been minimized.

Copy link
Owner

jwilm commented Nov 30, 2018

I think the ask was to run new files in the PR through rustfmt? This doesn't seem like it should be a problem if I'm understanding correctly.

@siler

This comment has been minimized.

Copy link

siler commented Nov 30, 2018

@davidhewitt

This comment has been minimized.

Copy link
Contributor

davidhewitt commented Dec 4, 2018

Agreed about running the new files through rustfmt. I'm also working on some further cleanups to the mio-anonymous-pipes code; will hope to push the lot by the weekend.

@davidhewitt

This comment has been minimized.

Copy link
Contributor

davidhewitt commented Dec 8, 2018

I've moved mio-anonymous-pipes out into its own crate and run conpty.rs through rustfmt. Last thing left on my list is to benchmark versus winpty.

@davidhewitt

This comment has been minimized.

Copy link
Contributor

davidhewitt commented Dec 9, 2018

Is there prior art in the repository for benchmarking the tty implementations? Everything I can find related to cargo bench inside alacritty is related to Term or other sections of the codebase which don't look like they'll exercise the tty. Perhaps I need to cook some new benchmarks up (maybe after #1653 is merged)?

@chrisduerr

This comment has been minimized.

Copy link
Collaborator

chrisduerr commented Dec 9, 2018

@davidhewitt I'm not sure how soon #1653 will be merged because it's not super clean and reliable. I also don't think there's any Rust benchmarking for the tty implementations, because most of the benchmarking is done through https://github.com/jwilm/vtebench.

@lzybkr

This comment has been minimized.

Copy link
Contributor

lzybkr commented Dec 13, 2018

This PR looks like a great speedup.

I followed the suggestion in https://github.com/jwilm/vtebench - I generated some output with:

vtebench -h 45 -w 110 -b 10485760 --term=xterm-256color alt-screen-random-write > 10mb.vte

Then ran time cat 10mb.vte 5 times and took the average:

Scenario Time
Alacritty w/ conpty 5.831s
Alacritty w/ winpty 9.330s
conhost 9.340s

I should add that I tested with a random internal build of conhost.exe that is similar to but not exactly any Windows Insiders build. This random conhost.exe did fix the flickering I saw with 1809.

Of note - Alactritty w/ winpty felt slower than conhost even thought it wasn't. With smaller inputs, I would say they felt similar though.

But it's clear - Alacritty w/ conpty is noticeably faster.

@davidhewitt

This comment has been minimized.

Copy link
Contributor

davidhewitt commented Dec 13, 2018

Thanks for running that @lzybkr! I had a brief run of a similar experiment on the 1809 conhost and found that the conpty variant was a bit slower than winpty (but it was close, and the framerate was weirdly significantly higher for the conpty variant despite the longer overall runtime). As you say the 1809 conhost has some flickering bugs (effectively adding overhead to the tty output as I understand it).

I was planning to look into that a little more before reporting back, but I think as your numbers show the insiders conhost (so presumably will make it to most users in 1904?) is so much better I'm less inclined to worry about benchmarking 1809 precisely. As the config option says, it's experimental anyway :)

@chrisduerr
Copy link
Collaborator

chrisduerr left a comment

Performance seems to be fine on Windows and there has been no regression on Linux either, so this looks mostly good to go to me.

I've just got a few comments left and it might be worth waiting for the winapi PR since I'd expect that to be merged soon-ish.

Is there anything left to do from your side?

Show resolved Hide resolved src/lib.rs Outdated
Show resolved Hide resolved src/tty/windows/mod.rs Outdated
Show resolved Hide resolved src/tty/windows/mod.rs
@zacps

This comment has been minimized.

Copy link
Collaborator

zacps commented Dec 14, 2018

So I finally got the chance to try this and it's looking great!

There are a couple of issues with winpty that I'd hoped would have been resolved by this but aren't unfortunately.

Given that it seems like both of the issues in the OP have been resolved I'd be happy to merge this without the experimental flag, given that the last stylistic changes are resolved.

@davidhewitt

This comment has been minimized.

Copy link
Contributor

davidhewitt commented Dec 15, 2018

I've just updated to latest master, have tidied up for Rust 2018 etc.

I've opened #1905 for the future evolution to overlapped I/O. I'll leave the TODO detail in the code as well.

Given that it seems like both of the issues in the OP have been resolved I'd be happy to merge this without the experimental flag, given that the last stylistic changes are resolved.

Sadly the zerowidth fix on master does not seem to have resolved the issue I occasionally have of characters ghosting in the last column. I'd like to investigate that further over the xmas break. It's a minor visual annoyance rather than a usability blocker though, your call if it's a blocker for merging (or maybe merge with experimental flag still?).

Either way we're still waiting on the winapi PR to be merged. I last pinged the maintainer a week ago, I'll give him a bit longer before I ping again...

@chrisduerr

This comment has been minimized.

Copy link
Collaborator

chrisduerr commented Dec 15, 2018

If the winapi PR is not going to be merged soon, I'm fine with merging this before it and just creating a follow-up for that. Doesn't make sense to block forever on that.

@davidhewitt

This comment has been minimized.

Copy link
Contributor

davidhewitt commented Dec 15, 2018

As far as I know it's ready to merge, just waiting for the maintainer to click the button...

@damnskippy

This comment has been minimized.

Copy link

damnskippy commented Dec 17, 2018

As far as I know it's ready to merge, just waiting for the maintainer to click the button...

Is it @jwilm that needs to click this button?

@chrisduerr

This comment has been minimized.

Copy link
Collaborator

chrisduerr commented Dec 17, 2018

@damnskippy We're talking about winapi here, not Alacritty.

@davidhewitt davidhewitt referenced this pull request Dec 17, 2018

Merged

Add ConPty APIs #699

@chrisduerr chrisduerr added this to the Version 0.2.5 milestone Dec 21, 2018

@chrisduerr

This comment has been minimized.

Copy link
Collaborator

chrisduerr commented Dec 23, 2018

Tested this myself and it seems to be working fine. @davidhewitt I'd propose to rebase this on top of master, then I'll review the code itself again to make sure everything's fine and then we'll just merge it without the upstream winapi patch.

@chrisduerr
Copy link
Collaborator

chrisduerr left a comment

I've checked the code again to make sure everything's okay before merging this and there have been a few more issues I've noticed.

There might be some more slight improvements that could be made, but I'd rather get this merged rather than bikeshedding over minor details forever.

This also still needs a change log entry before it can be merged.

Show resolved Hide resolved src/tty/windows/conpty.rs
Show resolved Hide resolved src/tty/windows/conpty.rs
Show resolved Hide resolved src/tty/windows/mod.rs Outdated
Show resolved Hide resolved src/tty/windows/mod.rs Outdated
@davidhewitt

This comment has been minimized.

Copy link
Contributor

davidhewitt commented Dec 24, 2018

Thanks, have fixed up a couple of those and given detail on some others - will make changes when I have a clearer idea of your preferred direction on those things.

I've also brought into the source the definition of HPCON from the winapi patch I've been using (that patch also adds the definitions of the ConPty APIs if we were linking statically, which we're not anyway). So we're no longer using the patched winapi either. I've added #1920 to track possibly updating winapi and using the definition of HPCON from there later.

@chrisduerr

This comment has been minimized.

Copy link
Collaborator

chrisduerr commented Dec 24, 2018

@davidhewitt Thanks for working around the patched winapi dependency issue and creating an issue to resolve this in the future.

Now all that's left is the remaining unresolved discussions and the CHANGELOG.md entry.

I would also appreciate it if you could rebase/merge this on top of master to resolve the Cargo.lock conflict. Just running a cargo update after the update should make sure that everything's using the latest versions possible.

@davidhewitt davidhewitt force-pushed the davidhewitt:consoleapi branch 2 times, most recently from f20ce82 to 2fce9f0 Dec 27, 2018

@davidhewitt

This comment has been minimized.

Copy link
Contributor

davidhewitt commented Dec 27, 2018

Removal of lazy_static, addition of Changelog entry, and rebase all complete. Probably worth a final read but otherwise I think we're ready to go! 😄

davidhewitt and others added some commits Dec 27, 2018

@chrisduerr chrisduerr force-pushed the davidhewitt:consoleapi branch from 56b490d to ff17d96 Dec 28, 2018

@chrisduerr
Copy link
Collaborator

chrisduerr left a comment

I've fixed some minor documentation issues myself and ran rustfmt over all new files and everything's looking great now.

Thanks for all the work. Going to wait for CI to finish and then test it manually one last time and then it should be ready to get merged!

@chrisduerr

This comment has been minimized.

Copy link
Collaborator

chrisduerr commented Dec 28, 2018

@davidhewitt It actually looks like #1920 has been resolved. Do you think it makes sense to fix that right now?

If you would prefer to do that after this PR has been merged, that would be fine too though. Just let me know.

Winapi is likely not getting a new version until around new years (at least that's what the maintainer told me), we could just build against git though. It's easier to switch a dep from git to fixed version than making big code changes. However that could lead to some breakage (which should be fine since we provide binaries ourselves though).

@chrisduerr chrisduerr merged commit f1bc680 into jwilm:master Dec 28, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@chrisduerr

This comment has been minimized.

Copy link
Collaborator

chrisduerr commented Dec 28, 2018

Decided to just merge this PR. Follow-ups are always possible. Better to get things out of the way quickly.

@davidhewitt

This comment has been minimized.

Copy link
Contributor

davidhewitt commented Dec 29, 2018

Excellent! I plan to wait for winapi release but ping me on #1920 if you'd prefer we switch to the winapi from git for now.

@davidhewitt davidhewitt deleted the davidhewitt:consoleapi branch Dec 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment