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

Keep slave program in sync with terminal #146

Closed
wants to merge 2 commits into from
Closed

Keep slave program in sync with terminal #146

wants to merge 2 commits into from

Conversation

jerch
Copy link
Member

@jerch jerch commented Jun 25, 2016

Since I got no anwser to #134 (comment) here is a PR addressing the out of sync problem of the slave program and the terminal emulator.

The problem boils down to the nonblocking websocket handling, which cant propagate back the "uhmm, I'm still busy, please wait" of the terminal emulator and buffers more and more bytes. In the result the emulator lacks far behind in data processing. Thus a long running output heavy task seems not to be interruptable (which is not the case, it is just the terminal emulator, which has to get through the big amount of data to the point where the interrupt happened).
This PR uses XON/XOFF flow control to keep the backend and frontend in sync.

You can test this with any long running output intensive task, e.g. ls -lR /

Related issues are #134 and microsoft/vscode#7801

@Tyriar
Copy link
Member

Tyriar commented Jun 25, 2016

While this does seem to work, and is quite snappy when sending SIGINT, I don't think we necessarily want to ensure that absolutely everything has been synced to the UI. While the frontend can be optimized further (as detailed in #134), this change negates the work that was done in #128 which allows as much data as possible through, we just only print the last page of it.

Consider this command with this change:

$ time ls -lR ~
...
real    3m21.946s                                                                                 
user    0m2.796s                                                                                  
sys     0m4.752s

And without the change:

$ time ls -lR ~
...
real    0m3.289s                                                                                  
user    0m1.124s                                                                                  
sys     0m2.056s

real is not actually accurate on the second output which is what is so awesome about this change, but the command only takes about 10 seconds to run.

Perhaps this could be hooked up somehow to refresh instead of write so that we can get the benefits of both the refresh throttling and XON/XOFF?

@jerch
Copy link
Member Author

jerch commented Jun 26, 2016

Ah yes your 30fps refresh patch will have no effect with all those xon/xoffs, since they will stop the backend with every websocket transfer. Although the stops bind any keyboard action to the next websocket action (therefore feeling snappy and in sync) it is not suitable - the average websocket transfer buffer is about 1k to 3k even for output heavy tasks, that means the backend gets stopped every few thousand bytes. Bummer.

I created a new branch with a different approach: https://github.com/jerch/xterm.js/tree/flowcontrol_buffer
Here I use a high/low-water mark buffer to trigger the flow control. The high mark is used to halt the backend (out of sync gap getting to big) and the low mark is used to avoid waiting time on the frontend side for new data.
All settings are adjustable, the commit contains the settings where even my FF can handle it smoothly (which is not the case with just your fps patch). SIGINT gets through at the average max-min mark (the buffer state shows a sawtooth behavior).

Regarding the xon/xoff at refresh time:
Imho there is much room for rendering optimizations which should be applied first. (The switch to pre with span nearly doubled the throughput of my emulator, same with fragments instead of HTML strings).

@parisk
Copy link
Contributor

parisk commented Jun 27, 2016

Sorry about the delay to get back on this, but finally I got my input. I am not quite fond of the idea of synchronizing the back-end with the front-end. IMO the best option is optimizing rendering as much as possible and maybe offering this feature as an add-on or a flag (sync?) to the attach add-on.

Also it would be great if you could provide us with a PR which swaps divs with pres, in order to try it out and see how does it perform.

@jerch
Copy link
Member Author

jerch commented Jun 27, 2016

Also it would be great if you could provide us with a PR which swaps divs with pres, in order to try it out and see how does it perform.

This would need an almost full rewrite of the refresh method. Gonna try it if I find some time for that.

@parisk
Copy link
Contributor

parisk commented Jun 28, 2016

Another thing we could do would be opening a GitHub issue, proposing a refactoring of the refresh method based on this principles, in order to create a place for discussion and make it available for anyone to take it over.

Do you think you could do that (creating a GitHub issue)?

@jerch
Copy link
Member Author

jerch commented Jun 28, 2016

Sure - feel free to add more ideas and discuss it here #150

@parisk
Copy link
Contributor

parisk commented Jun 29, 2016

Awesome, thanks!

@jerch
Copy link
Member Author

jerch commented Jul 12, 2016

Gonna close the PR since it is obsolete atm...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants