-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Proposal: revamp the refresh method for better rendering performance #150
Comments
While canvas may be more performant, getting ti to act like a text editor may be tricky (selection, context menus, IMEs, etc.), also it's a very radical change that makes it behave significantly different. It would be good if there was separation between the rendering and terminal state, a renderer could then be swapped in or out allowing for choice. Not really something that needs attention immediately though. The first 2 points will have by far the biggest impact imo, that's where we should focus. Resolving point 2 should also fix the layout thrashing (point 5). I still plan on picking up the document fragment change when I get some time away from vscode stabilization (1-2 weeks). Also on native scrolling, this is only possible if the entire buffer is created in the DOM. This would be an enormous memory footprint for little payoff. |
My two cents here:
My proposal on this is first try to replace |
I second your remarks about the canvas idea. Too much of an effort for an uncertain result. Might be worth a look again once a bigger overhaul is needed, but not at the current state. Imo missing in the list above - CSS optimization. Here is a nice overview of CSS concerns. Atm there are two selectors that might be performance critical - Gonna go for some |
Native scrolling might be possible in combination with partial buffer rendering upon scrolling. Clusterize.js uses such a trick to show tons of data in a list. |
@jerch I don't think those selectors should be particularly badly performing. Similar with |
As far as I understand the concerns the evaluation from right to left of the selector makes them counterintuitive. For But the referred document is pretty old, I am not sure, if it is still that bad (http://calendar.perfplanet.com/2011/css-selector-performance-has-changed-for-the-better/) Since I dont know how to test the CSS stuff reliably, I would not spend to much time for it. |
Now I feel like Don Quixote tilting at windwills - ever wondered what this greyish pie in dev tools timeline summary is? It is "Other". Hmm. I ran several tests with different settings to get better output performance and was able to lower the total runtime of Dramatic prologue, short answer - websockets are slow. I switched off all the frontend handling (no parsing, no output) to see the raw performance. By playing with the server side buffer size I was able to raise the throughput to 3.5 MB/s. Seems the websocket stack itself is to heavy to cope with many short data and would benefit from buffering before sending. current master: websocket buffered (plus some other rendering changes): Change: |
Did some tests with Another possible optimization - the terminal container |
Thanks a lot for all these information @jerch. They are really useful. I will take a deeper look later, because I couldn't find the time today. Could you please provide us with some more information about the websocket server you used and the settings you used to set up buffering? |
@parisk You can find the adjustments here. The websocket buffer is just a quick hack to your app.js to see the performance difference. The changes should be applicable to any websocket lib (in fact the buffer is before websocket invocation), though Im not sure if it will show such an impact on a remote system, comm latency might just swallow the effect idk. Also the separation of the terminal inner state handling and the rendering might be a goal (as Tyriar mentioned above). This would give other optimization options like using web workers for not DOM related work. I did a quick&dirty test with one worker thread for websocket comm and state handling and sole rendering in the main thread with these results:
|
@jerch thanks for providing all this data. After releasing 1.0.0 this week, I will get on top of this, in order to set up a "roadmap" or course of action in order to tune the rendering process. |
Looking at this now after the changes I've done as part of microsoft/vscode#17875 I don't think it's worth pursuing any of these points. While caching would be nice, it comes at the cost of potentially quite significant memory overhead and performance is fine as is imo. Caching is less likely to be needed now also due to the queue change (#438) which will likely see the entire viewport change a lot more often than individual rows in which case caching would only help scrollback at the cost of lots of memory. |
To start the discussion about the screen representation and rendering process here are some first ideas:
pre
container instead ofdiv
pre
context can be rendered faster by most browser engines due to the reduced capabilities. Downside - reduced capabilities ;)Although this needs more JS code with all the explicit node manipulation it is super fast compared to
innerHTML
. IMHO a must-do ;)The emulator is caching real DOM nodes at line level at the moment. Maybe another caching strategy in combination with fragments might lift the burden to some degree.
This is a hard one, not sure if this is worth the trouble at all. Would give the full power of what is going to be shown. Downside - needs tons of JS code to get the layouting done and all those nifty events we get with DOM nodes for free. No clue if it will show better performance in the end since text rendering on canvas is not that good. Maybe someone has experience with that, therefore open for discussion...
Those calls will almost always trigger an incremental synchronous re-layouting (full stop until reflow is done). Needs to be checked...
Somewhat offtopic but still performance relevant:
What about native scrolling? Other ideas?
Some unicode chars will break the terminal grid. Needs some CSS rules, maybe even some kind of font glyph measuring tricks.
The text was updated successfully, but these errors were encountered: