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.

Sign up for GitHub

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

Jump to bottom

_clipImageData is GC expensive #4197

Closed
Tyriar opened this issue Oct 11, 2022 · 4 comments · Fixed by #4200 or #4201
Closed

_clipImageData is GC expensive #4197

Tyriar opened this issue Oct 11, 2022 · 4 comments · Fixed by #4200 or #4201
Assignees
@Tyriar
Labels
area/performance type/enhancement Features or improvements to existing features
Milestone
5.1.0

Comments

@Tyriar
Copy link
Member

Tyriar commented Oct 11, 2022

Part of microsoft/vscode#161622

During vscode startup:

Screen Shot 2022-10-11 at 9 18 05 am
@Tyriar Tyriar added type/enhancement Features or improvements to existing features area/performance labels Oct 11, 2022
@Tyriar Tyriar added this to the 5.1.0 milestone Oct 11, 2022
@Tyriar Tyriar self-assigned this Oct 11, 2022
@Tyriar
Copy link
Member Author

Tyriar commented Oct 11, 2022

This was when connecting to existing terminals with existing content which is hard to reproduce on the demo. A close approximation is the load test button, see the memory chart:

Screen Shot 2022-10-11 at 10 10 17 am

@Tyriar
Copy link
Member Author

Tyriar commented Oct 11, 2022

Actually this might be an issue with the whole draw to cache call, closing for not until I understand the problem better and whether it's worth fixing.

@Tyriar Tyriar closed this as completed Oct 11, 2022
@Tyriar Tyriar added the invalid label Oct 11, 2022
@jerch
Copy link
Member

jerch commented Oct 11, 2022

Looking at the code of that function, I think you can spare the new Uint8ClampedArray there by directly creating the ImageDate and writing to its .data? Would atleast lower the gc pressure somewhat.

The bytes also could be transferred as uint32, would make the function somewhat faster.

@Tyriar
Copy link
Member Author

Tyriar commented Oct 11, 2022

Great idea, then it's just a new typed array view on the same buffer until we eventually throw it away after it gets drawn to the canvas!

@Tyriar Tyriar reopened this Oct 11, 2022
@Tyriar Tyriar removed the invalid label Oct 11, 2022
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Oct 12, 2022
@Tyriar
Optimize _clipImageData
28fcffe 
This makes clip image data around twice as fast, it's a little tricky to
measure it though. Changes:

- Work on Uint32Array instead of Uint8 so it's 1 assignment per pixel
  instead of 1 per channel.
- Perform the clipping in-place using the original image data to avoid
  allocation a new buffer.

Fixes xtermjs#4197
@Tyriar Tyriar mentioned this issue Oct 12, 2022
Merged
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Oct 13, 2022
@Tyriar
Remove clipImageData completely
50d1834 
putImageData works a little differently to drawImage which confused me
for a bit, you need to offset the destination as putImageData draws the
'dirty' parts of the texture using the same source image dimensions

Fixes xtermjs#4197
@Tyriar Tyriar mentioned this issue Oct 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Tyriar Tyriar

Labels
area/performance type/enhancement Features or improvements to existing features
Projects
None yet
Milestone
5.1.0
Development

Successfully merging a pull request may close this issue.

Optimize _clipImageData Tyriar/xterm.js
Remove clipImageData completely Tyriar/xterm.js
2 participants
@Tyriar @jerch