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

Mass decoration registration and disposable is slow #4911

Closed
Tyriar opened this issue Dec 11, 2023 · 6 comments · Fixed by #5038
Closed

Mass decoration registration and disposable is slow #4911

Tyriar opened this issue Dec 11, 2023 · 6 comments · Fixed by #5038
Assignees
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Dec 11, 2023

VS Code issue: microsoft/vscode#199593

Lots of GC happening when registering and disposing many decorations. Here's an example showing and hiding a command guide in VS Code which creates and then disposed ~16k decorations:

image
image

https://github.com/Tyriar/xterm.js/blob/1943636f023b0496c2b95aeb31f2d08d7c0e4dce/src/common/services/DecorationService.ts#L41-L60

Some ideas to speed up:

  • Don't use splice(!) in SortedList
    • Doing something more clever here would probably vastly improve registration
  • Use an object pool
  • Defer clean up to idle callback
@jerch
Copy link
Member

jerch commented Dec 11, 2023

My guess also goes to the splice needs here, SortedList basically implements the insert sort idea, which is nice for insertion/removal at the end with O(1), but soon saturates to O(n²) for average (I guess this happens alot here, when lines drop off the scroll buffer at the top).

Edit: For reference - I've done an RB tree impl back during the buffer rewrite (which never made it into the core code), maybe this comes handy at some point: https://github.com/jerch/xterm.js/blob/AttributeStorage/src/AttributeStorage.ts
(sidenote - the code is not nicely abstracted but highly optimized for the attribute task back then 😅 )

Edit2: A balanced tree based impl will perform alot worse for typical insertion/removal at the end. Maybe partitioning of the list (like a skiplist) will already do the job here?

@Tyriar
Copy link
Member Author

Tyriar commented Dec 11, 2023

Yes RB or AVL is probably the fix. I've done both before but it looks like I never bothered porting the RB tree from js to ts yet. https://github.com/gwtw/ts-avl-tree, https://github.com/gwtw/js-data-structures/blob/master/lib/red-black-tree.js

A balanced tree based impl will perform alot worse for typical insertion/removal at the end. Maybe partitioning of the list (like a skiplist) will already do the job here?

It's still just log n though, the main thing that matters is that it scales well imo

Tyriar added a commit to microsoft/vscode that referenced this issue Dec 18, 2023
This can come back after xtermjs/xterm.js#4911 is handled.

Fixes #199593
@tisilent
Copy link
Contributor

tisilent commented Feb 5, 2024

I see if there is anything I can do on the demo to facilitate subsequent verification. 😀

@jerch
Copy link
Member

jerch commented Feb 6, 2024

@Tyriar How would an easy repro look like? Is it enough to spam tons of decorations all over the lines, or do they need to follow a certain pattern?

Which action triggered the toxic removals? Was that just from scrolling off at the top? Because if thats the case, we could prolly lift some burden here by batching the removals with interim "dead keys" on the sorted list and doing a final cleanup copy. Difference would be O(n + m log n) vs. currently O(m*n + m log n) for m elements to be removed from n elements (the m multiplier comes from the splice copy overhead for every single removal in our current bisearch&splice approach, which we have only once with dead keys).

@Tyriar
Copy link
Member Author

Tyriar commented Apr 20, 2024

There's a lot of decorations in vscode now with shell integration, especially when you're hovering the terminal as there's a "command guide" as well:

image

I think my repro above was something like running tree and then scrolling up with smooth scroll on. Doing a find will make it more intense

@Tyriar Tyriar self-assigned this Apr 20, 2024
@Tyriar Tyriar added this to the 5.6.0 milestone Apr 20, 2024
@Tyriar
Copy link
Member Author

Tyriar commented Apr 20, 2024

Example with the decoration stress test button I made:

image

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