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

Optimize gc from decoration-related hot code paths #4080

Merged
merged 3 commits into from
Aug 28, 2022

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Aug 28, 2022

Fixes #4079

Before:

image

After:

image

@Tyriar Tyriar added this to the 5.0.0 milestone Aug 28, 2022
@Tyriar Tyriar self-assigned this Aug 28, 2022
@Tyriar Tyriar merged commit eb31850 into xtermjs:master Aug 28, 2022
@Tyriar Tyriar deleted the decoration_gc branch August 28, 2022 14:44
@jerch
Copy link
Member

jerch commented Aug 28, 2022

@Tyriar Maybe related (and maybe already fixed?) - when doing some perf benchmarks with canvas renderer I found this to be quite expensive (up to 25% of the renderer runtime):

this._decorationService.forEachDecorationAtCell(x, this._bufferService.buffer.ydisp + y, undefined, d => {

I did not look at the code in detail, so I dont know what causes the high runtime for sure. I saw that there are some iterators/generators, maybe those are already the culprit? (They were a real showstopper in the past, but I think engines got alot better in handling those in a speedy manner). Since I had no decorations in place at all I wonder if there is at least an early fast exit possible to cut off most of the code path.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 28, 2022

@jerch it should be much faster now that it's forEach instead of the old iterator. The overhead was creating 2 iterators for each cell getting updated. Lovely to use but way too slow to create in this hot code path.

I've finished all the gc-related fixes if you want to re-try any tests.

@jerch
Copy link
Member

jerch commented Aug 29, 2022

@Tyriar Oh well, I meant the getDecorationsAtCell above (wrongly linked to the new impl). The forEach impl is indeed alot faster (<<5% of renderer runtime), which is somewhat sad, as the yield code reads much nicer (at least for me coming from python).

The w object is an interesting hack around the gc - is that still needed with the forEach way? Would have thought, that the gc can simply fast collect it here not creating any overhead (other than with generators, where the shadowed stack frames created from yield state add more overhead for the gc).

Anyway - perf issue is fixed 👍

@Tyriar
Copy link
Member Author

Tyriar commented Aug 29, 2022

The work object is a common trick we already used, just new bits were added since. I don't know exactly what variables are optimized away, but for (int i...) for example ends up causing minor GCs

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.

A large amount of CPU time is spent in loadColorsForCell
2 participants