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

Create a node-pty host process with flow control and event batching #74620

Closed
Tyriar opened this issue May 30, 2019 · 10 comments · Fixed by #116373
Closed

Create a node-pty host process with flow control and event batching #74620

Tyriar opened this issue May 30, 2019 · 10 comments · Fixed by #116373
Assignees
Labels
feature-request Request for new features or functionality freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues insiders-released Patch has been released in VS Code Insiders on-testplan perf terminal General terminal issues that don't fall under another label
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented May 30, 2019

As of around a year ago node-pty is launched directly from the renderer process, while this does mean ctrl+c is responsive it has several downsides:

My proposal for these problems:

@Tyriar Tyriar added feature-request Request for new features or functionality freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues terminal General terminal issues that don't fall under another label perf labels May 30, 2019
@Tyriar Tyriar added this to the On Deck milestone May 30, 2019
@Tyriar Tyriar self-assigned this May 30, 2019
@reinux

This comment has been minimized.

@Tyriar

This comment has been minimized.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 13, 2019

Commit that moved the process into renderer: 35cee02

@Tyriar
Copy link
Member Author

Tyriar commented Sep 17, 2019

Commit that converted to use amd ed9cdf5

@Tyriar
Copy link
Member Author

Tyriar commented Sep 18, 2019

Did some exploration into this.

How terminal processes are launched currently

                     TerminalInstance
                           |
                           v
                 TerminalProcessManager
                    |              |
                    v              v
TerminalProcessExtHostProxy    TerminalProcess
             |                 (renderer proc)
             v
   (an ext host connection)
             |
             v
      TerminalProcess
      (ext host proc)

Issues:

  • In proc node-pty can crash the renderer when conpty/winpty misbehave
  • Slow node event loop means slower data flow from the pty, I believe this is the primary reason terminals are a lot slower on Windows

Using just the ext host

          TerminalInstance
                 |
                 v
        TerminalProcessManager
                 |
                 v
    TerminalProcessExtHostProxy
                 |
                 v
(local or remote ext host connection)
                 |
                 v
          TerminalProcess

Issues:

  • In proc node-pty can crash the ext host when conpty/winpty misbehave
  • More data from terminal can flood ext host connection, eg. after running yes command (flow control is eventual solution to this)
  • Blocker: SSH extension depends on terminals which means the local ext host would have to be up and running

Prototype: #81106

Moving the processes out of proc, this is how it used to be (before ext host processes)

        TerminalInstance
               |
         (forked process)
               v
        TerminalProcess
        (uses node-pty)

Issues:

  • Slower data flow as we're passing around JS strings
  • Can flood connection (note this is not an issue locally because the nodejs event loop blocks it)

Proposed solution

                     TerminalInstance
                           |
                           v            
                 TerminalProcessManager 
                    |              |    
                    v              v    
TerminalProcessExtHostProxy    TerminalPty <--> TerminalInstanceService[2]
             |                                             |
             v                                       (forked proc[1])
   (an ext host connection)                                v
             |                                   TerminalPtyHostProcess
             |
             v                     
        TerminalPty <--> ExtHostTerminalTerminalService[2]
                                       |
                                 (forked proc[1])
                                       v
                             TerminalPtyHostProcess

[1] Uses ext host socket-based message protocol
[2] The terminal services own the host process and establish the connection

  • Crash protection for ext host and renderer, if node-pty crashes it would take down only TerminalPtyHostProcess (and all terminals it's running)
  • Local terminals are still available for SSH extension
  • Data flow
    • Faster data flow as messages passed over socket are binary data which node-pty can now emit and xterm.js can consume
    • Add flow control which pauses node-pty sockets when they are getting too ahead of xterm.js parsing of the data
    • TerminalPtyHostProcess is basically just handling data flow for all terminals, so it shouldn't block the pty as much
    • Can also batch data to reduce the amount of messages coming

Prototype: #81105

Note that this could be split into 2 stages:

  1. Moving into new process, communicate via ChildProcess.send and add flow control
  2. Adding new protocol and enabling fast transfer of data via ArrayBuffers

On flow control

We recently exposed a callback on xterm's write which fires when the data is parsed. This will make flow control much easier that the earlier proposals I talked about. For example we could provide a callback for event nth write and send messages over to TerminalPtyHostProcess to indicate where the renderer is at in parsing the data.

@Tyriar Tyriar mentioned this issue Oct 21, 2019
Tyriar added a commit to microsoft/node-pty that referenced this issue Jun 9, 2020
Conpty can send a final screen update after ClosePseudoConsole which waits for the socket
to be drained before continuing. When both the socket draining and closing occurs on the
same thread a deadlock can occur which locks up VS Code's UI, the only ways to fix are
to close the client or kill the misbehaving conhost instance. This change moves the
handling of the conout named pipe to a Worker which bumps the minimum Node version
requirement to 12 (10 with a flag). This change may also have positive performance
benefits as called out in microsoft/vscode#74620

Fixes #375
Tyriar added a commit to microsoft/node-pty that referenced this issue Jun 9, 2020
Conpty can send a final screen update after ClosePseudoConsole which waits for the socket
to be drained before continuing. When both the socket draining and closing occurs on the
same thread a deadlock can occur which locks up VS Code's UI, the only ways to fix are
to close the client or kill the misbehaving conhost instance. This change moves the
handling of the conout named pipe to a Worker which bumps the minimum Node version
requirement to 12 (10 with a flag). This change may also have positive performance
benefits as called out in microsoft/vscode#74620

The worker change also happens for winpty, it seems to work there too.

Fixes #375
@Tyriar
Copy link
Member Author

Tyriar commented Jun 11, 2020

I did an exploration into moving node-pty into the main process (#99897) and it worked pretty well. It's just a prototype and falls over when it comes to pty lifecycle management but it seems like it would be quite easy to tweak it to restore terminals across reloads #20013 if it lived in the main process (as the prototype shows). Another benefit is it speeds up the terminal output significantly, tree running in the same directly was several times faster due to event loop being freed up, I expect this to also happen when we get microsoft/node-pty#415 in, even when node-pty is hosted in the renderer.

My plan is to eventually move node-pty into a process that's forked from the main process and then establishing a direct socket connection for the terminal if possible to the window so we keep pretty much everything of the main process except for the setting up of the connection.

The branch in action with node-pty hosted on the main process and (the hacky) reload persistence:

recording (18)

@Tyriar
Copy link
Member Author

Tyriar commented Jan 5, 2021

I split the flow control and event batching portion of this into #113827

@Tyriar
Copy link
Member Author

Tyriar commented Feb 11, 2021

Update:

#116373 will be merged soon which will:

  • Create a ptyHost process off the shared process where node-pty is used
    • This should boost terminal performance on Windows due to conpty's small event sizes
  • Update node-pty to fix the Windows hang issue now that it's on a plain node process

I'll close this issue when the above gets merged and defer the following to more targeted issues:

@Tyriar
Copy link
Member Author

Tyriar commented Feb 18, 2021

This is what the process architecture ended up looking like:

Screen Shot 2021-02-18 at 5 37 39 AM

PtyService on the pty host process is what creates the terminal processes

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues insiders-released Patch has been released in VS Code Insiders on-testplan perf terminal General terminal issues that don't fall under another label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@Tyriar @tmandry @reinux @meganrogge and others