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

Performance: pause / resume rendering #880

Closed
mofux opened this issue Aug 12, 2017 · 8 comments
Closed

Performance: pause / resume rendering #880

mofux opened this issue Aug 12, 2017 · 8 comments
Labels
type/enhancement Features or improvements to existing features type/proposal A proposal that needs some discussion before proceeding

Comments

@mofux
Copy link
Contributor

mofux commented Aug 12, 2017

At the moment, the renderer will always draw incoming data (manipulate the dom), no matter if the terminal is actually visible to the user. Bigger applications like hyper or vscode, that may render multiple terminals in tabs would benefit a lot (performance and battery drain) if they could suspend the rendering while the terminal element is visually hidden. I think we should offer an API that allows the implementor to pause and resume the renderer.

term.pauseRenderer();
term.resumeRenderer();
term.rendererPaused;

Once the renderer is resumed, it has to immediately redraw to catch up the new state.
Any thoughts? I would be tempted to provide a PR 😉

@lucat1
Copy link
Contributor

lucat1 commented Aug 12, 2017

This would be amazing for my use case. I'm already doing something similar for the resizing: i resize just the focused terminal, and when the user switches to another tab the new terminal updates its geometry. This would be an awesome addition to improve it! 🎉

Maybe this could be an addon as not all use cases would benefit/need this?

+1

@mofux
Copy link
Contributor Author

mofux commented Aug 12, 2017

@lucat1 I think the API has to go into core because it needs to touch the internals of the renderer - but we could write a plugin that uses the IntersectionObserver to automatically pause and resume the renderer using the new API 🤓

@Tyriar
Copy link
Member

Tyriar commented Aug 12, 2017

I don't think this is as big of a problem as it first sounds like as in general, at least for me, background tabs aren't doing much and therefore aren't refreshing much. Seems like a good area for investigation though.

I'm not much of a fan of the low level API, but it would be really cool if we just handled everything if the browser supports IntersectionObserver. I wonder if registered IntersectionObservers doing a whole bunch of work whenever scrolling happens that could negatively impact performance as well?

@mofux
Copy link
Contributor Author

mofux commented Aug 12, 2017

@Tyriar In my case, I manage a lot of servers, having many tabs (ssh connections) open to them, and when troubleshooting I have to do a lot of tail -f logfile in multiple tabs.

With a fullscreen terminal and five fullscreen tabs running tail -f busy-log-file.log you can feel performance degrading quite a lot, which made me come to this idea. Note that there is quite a difference in performance between running five terminals split on the same screen, or if they were running all in separate tabs. Because on the same screen they still only occupy all the pixels on the screen once, but with one tab active and four fullscreen tabs in the background, they occupy 5* screen size (and cols and rows).

There was a nice post from one of the authors of the atom editor a while ago, that describes how they moved to use IntersectionObserver and ResizeObserver (here are the few lines of code that do it: https://github.com/atom/atom/blob/4b2a26e74f8290f7206e6ca5706fb2a51bc04200/src/text-editor-component.js#L1344). Btw. the ResizeObserver is now enabled in stable Chrome 60 by default and we could also use it for the fit addon!

Performance wise I can only guess, but since it is somewhat baked into the browser's rendering pipeline I would assume it should be a really minimal impact.

Last but not least, xterm this: telnet mapscii.me

@parisk parisk added type/enhancement Features or improvements to existing features type/proposal A proposal that needs some discussion before proceeding labels Aug 16, 2017
@parisk
Copy link
Contributor

parisk commented Aug 16, 2017

This is quite interesting.

I think that before deciding to move on with this, we should have some benchmarks or at least some proof that such a change will have a performance or energy consumption impact.

Now as far as considering the API, I would be happy if we exposed a limited public renderer API.

Example:

let term = new Terminal(...);

term.renderer.status;
term.renderer.pause();
term.renderer.resume();

My biggest implementation question is how do we store what is the last state that got rendered from the renderer and from where should we start rendering again.

EDIT: Maybe we could tag buffer lines with a unique ID. Not sure about implications though.

@Tyriar
Copy link
Member

Tyriar commented Aug 16, 2017

My biggest implementation question is how do we store what is the last state that got rendered from the renderer and from where should we start rendering again.

The last state of the renderer will still be in the DOM, all this change would be in its simplest form is some boolean flag which is checked by the renderer and the renderer does nothing. resume would flip the switch back and trigger a full refresh.

@mofux
Copy link
Contributor Author

mofux commented Aug 16, 2017

I'll create a proof-of-concept PR with a modified demo that creates multiple terminals that get filled with data continuously and an option to enable / disable the renderer for every single terminal. That way we should be able to inspect if performance is positively impacted.

@Tyriar
Copy link
Member

Tyriar commented Aug 16, 2017

@mofux I'd love to have a proper set of benchmarks checked in that we could run from the command line. I had a go at it earlier using phantomjs https://github.com/Tyriar/xterm.js/tree/phantomjs but I didn't end up polishing it and then phantomjs was abandoned in favor of headless chrome. If you want to explore that, it's an area that needs help too 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Features or improvements to existing features type/proposal A proposal that needs some discussion before proceeding
Projects
None yet
Development

No branches or pull requests

4 participants