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

Improve cluster construction performance #5584

Merged
3 commits merged into from
Apr 28, 2020

Conversation

skyline75489
Copy link
Collaborator

Summary of the Pull Request

A tiny performance fix in renderer.cpp.

References

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

The cluster construction code is intensively called during rendering. Even though a single back() is fast, but accumulated back()s still take a noticiable amount of CPU cycles.

Before:

perf1

After:

图片

Validation Steps Performed

Manually validated.

@skyline75489
Copy link
Collaborator Author

The ultimate solution may be something like #3458 . But that PR isn't going to land any time soon. AFAIK there's lots of things need to be done before landing something close to #3458.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

This looks fine to me but I'd want to make sure that @miniksa signs off on this. Thanks!

@zadjii-msft
Copy link
Member

@msftbot make sure that @miniksa signs off on this

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 27, 2020
@ghost
Copy link

ghost commented Apr 27, 2020

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@zadjii-msft zadjii-msft added Area-Performance Performance-related issue Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Terminal The new Windows Terminal. labels Apr 27, 2020
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Approved with the comment I merged in here to remind us what the variable is for.

@ghost ghost merged commit 97d6456 into microsoft:master Apr 28, 2020
@ghost
Copy link

ghost commented May 5, 2020

🎉Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1) has been released which incorporates this pull request.:tada:

Handy links:

@DHowett
Copy link
Member

DHowett commented Jul 2, 2020

🎉 Once again, thanks for the contribution!

This pull request was included in a set of conhost changes that was just
released with Windows Insider Build 20161.

@skyline75489 skyline75489 deleted the fix/vector-back-overhead branch September 22, 2020 10:33
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Performance Performance-related issue Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues AutoMerge Marked for automatic merge by the bot when requirements are met Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants