-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Faster DOM renderer #4605
Faster DOM renderer #4605
Conversation
Looking forward to this one! 😀 |
Geez, the branch/commit history is very broken (prolly from my older attempts), needs serious rebasing... |
@willmcgugan Indeed, this promises some serious perf boost for DOM renderer. Still it creates quite some glitches, idk yet if they all can be worked around. |
@Tyriar Plz have a look at this profiler stats: Is there a specific reason, why the selection service calls into |
@Tyriar The last commit can fix the superfluous paints from the selection service, saving ~1s runtime for my test. Would be good if you can have a look at it and check, whether thats a viable way to debounce the selection refreshs. |
Eww - the link underline logic for mouse hovers needs a full rework with the merged spans, it simply cannot be done that way anymore 😢 Edit: Only way I found to solve this is by explicit re-rendering of the rows in the link range, not nice but gets the job done. |
@Tyriar The following test is failing on windows only for no obvious reason: xterm.js/addons/xterm-addon-search/test/SearchAddon.api.ts Lines 245 to 261 in 1c98037
Tested it manually under linux, where it works as expected. Since I have no working windows installation I cannot look into it. Any clue why this might be failing out of a sudden? |
About ligature joins (manually tested):
(Sidenote: The current DOM and webgl renderer also have glitches for ligatures, maybe caused by something else. Also the issues dont look major/blocking to me.) |
I'd say this implementation is ready for first alpha tests. It would be very helpful if anyone interested in testing this could run it with any sort of complicated cmdline tools, so we can spot left-over regressions. |
Not sure yet what to do about the bold, italic and bold&italic font variants. Currently it is assumed, that a proper monospace font wont have different glyph widths for those, but this gonna break really bad if only the regular variant is available and the font renderer has to use a fallback font. The issues with also calculating these variants are:
Possible solutions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it a good test and it works very well for me (Linux with Edge and Firefox). The performance is impressive - well done! It's really only the block characters where the WebGL renderer is still superior. There certainly are ways to also add those custom-drawn block elements to the DOM renderer, but that is a challenge for another day :)
@mofux Yeah the performance is quite impressive for the fact, that it is still DOM manipulations going on. Thx again for the letter-spacing trick, it is def. the better way to deal with it. As noted above, there are ~20% more perf bonus possible, if we manage to get the spans into About the block chars - yeah kinda all font renderer libs have issues with those in monospace, as far as I am aware almost all other TEs render those themselves to overcome the font lib shortcomings. I have not thought about a workaround for the DOM renderer yet, surely we can find here something. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a thorough review, looks close
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a thorough review, looks close
Also there's a conflict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like everything is checked off and comments resolved so let's merge and I'll pull it into VS Code so we can get some early testing. @jerch can you make the followup issues when you get a chance?
Yeah all done. 😺 |
Fixes #4604.
TODO:
investigate, whether- Postponed for own PR.inline-block
on spans is really needed (+20% render penalty)avoid full re-render in- Postponed.handleSelectionChanged
inline-block
vs.inline
--> inline-block vs inline in DOM renderer #4632handleSelectionChanged
logic --> better handleSelectionChanged in DOM renderer #4633Also fixes #4554.
Also fixes #4602.
Also fixes #4601.
Also fixes #4556.