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

Dynamic atlas: Reduce unnecessary objects generated and draw from ImageBitmap #1692

Merged
merged 18 commits into from Sep 23, 2018

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Sep 14, 2018

This PR removed several of the in-between objects needed to render using the dynamic texture atlas. It's a bit difficult to measure this currently (#1688), the one piece that's very easy to measure is the impact on using a number instead of a string for the cache key:

Before:

screen shot 2018-09-14 at 12 11 50 pm

After:

screen shot 2018-09-14 at 12 12 02 pm

I couldn't find anything else that could contribute to improving #1677. I also experimented with drawing using an ImageBitmap but couldn't find big changes in performance so I left it out of the PR: Tyriar@e8d06b6

Fixes #1677

@Tyriar Tyriar self-assigned this Sep 14, 2018
@Tyriar Tyriar requested a review from bgw September 14, 2018 19:14
@jerch
Copy link
Member

jerch commented Sep 14, 2018

@Tyriar
Benchmarking with my ls -lR /usr/lib (average of 10 runs):

  • master: ~850ms for TextRenderLayer._forEachCell
  • this PR: ~750ms for TextRenderLayer._forEachCell
  • your 1677_bitmap branch: ~550ms for TextRenderLayer._forEachCell

I see a really big difference with the ImageBitmap on Ubuntu with Chrome 68 and nvidia gc. Welcome to browser version + driver differences? Idk.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 15, 2018

@jerch oh yeah? I felt like I saw that earlier and then didn't... I can continue polishing that up with the fallback if it looks consistent.

@Tyriar Tyriar added the work-in-progress Do not merge label Sep 15, 2018
@Tyriar Tyriar changed the title Reduce unnecessary objects generated when drawing from the dynamic text atlas Dynamic atlas: Reduce unnecessary objects generated and draw from ImageBitmap Sep 15, 2018
@Tyriar Tyriar removed the work-in-progress Do not merge label Sep 15, 2018
@jerch
Copy link
Member

jerch commented Sep 16, 2018

@Tyriar: Retested, its stable between 430 - 550 ms now 👍

Copy link
Contributor

@bgw bgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the removal of IGlyphIdentifier should be split into a separate PR since we don't have clear performance numbers for it yet, and it makes the code a lot uglier.

Otherwise, I think this is a good direction. Thanks!

src/renderer/atlas/DynamicCharAtlas.ts Outdated Show resolved Hide resolved
src/renderer/atlas/Types.ts Outdated Show resolved Hide resolved
src/renderer/atlas/LRUMap.ts Show resolved Hide resolved
src/renderer/atlas/DynamicCharAtlas.ts Show resolved Hide resolved
src/renderer/atlas/DynamicCharAtlas.ts Outdated Show resolved Hide resolved
@Tyriar
Copy link
Member Author

Tyriar commented Sep 17, 2018

@bgw thanks for the review! I marked all the conversations as resolved, let me know if you have more feedback (and feel free to unresolve if you don't think they are 😃).

src/renderer/BaseRenderLayer.ts Show resolved Hide resolved
src/renderer/atlas/DynamicCharAtlas.ts Outdated Show resolved Hide resolved
src/renderer/atlas/LRUMap.ts Show resolved Hide resolved
src/renderer/atlas/DynamicCharAtlas.ts Outdated Show resolved Hide resolved
@Tyriar
Copy link
Member Author

Tyriar commented Sep 20, 2018

Ready for another round 😃

Copy link
Contributor

@bgw bgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work, I know there was a lot of back-and-forth here, but I'm pretty happy with the result.

To help test some of this, I'd recommend setting the DynamicCharAtlas's size to something really small, so you can verify that we still behave correctly when the cache is being thrashed.

src/renderer/atlas/DynamicCharAtlas.ts Outdated Show resolved Hide resolved
src/renderer/atlas/DynamicCharAtlas.ts Outdated Show resolved Hide resolved
src/renderer/atlas/DynamicCharAtlas.ts Outdated Show resolved Hide resolved
src/renderer/atlas/DynamicCharAtlas.ts Outdated Show resolved Hide resolved
@Tyriar
Copy link
Member Author

Tyriar commented Sep 22, 2018

@bgw thanks again for all the feedback, we're in a much better place than at first. A once over of the last couple of commits would be great before I merge 🙂

@bgw
Copy link
Contributor

bgw commented Sep 22, 2018

LGTM, thanks. Did you test it with a small atlas size to verify that there's no obvious bugs when the atlas gets thrashed?

@Tyriar
Copy link
Member Author

Tyriar commented Sep 23, 2018

@bgw yes, no errors when I went down to a 100x100 texture.

@Tyriar Tyriar added this to the 3.8.0 milestone Sep 23, 2018
@Tyriar Tyriar merged commit 9e446a9 into xtermjs:master Sep 23, 2018
@Tyriar Tyriar deleted the 1677_perf_improve branch September 23, 2018 17:41
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.

None yet

3 participants