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

Reuse text layout for performance #2980

Closed
wants to merge 1 commit into from
Closed

Reuse text layout for performance #2980

wants to merge 1 commit into from

Conversation

skyline75489
Copy link
Collaborator

Summary of the Pull Request

Another possible performance issue here.

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

This is a draft. Just showing some idea.

Validation Steps Performed

zadjii-msft
zadjii-msft reviewed Sep 30, 2019
View reviewed changes
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 seems reasonable to me, but I'd definitely want @miniksa to sign off on it.

@zadjii-msft zadjii-msft added the Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues label Sep 30, 2019
@skyline75489
Copy link
Collaborator Author

I'm thinking there should be some time that _drawingTextLayout need to be recreated. Just not so sure about when.

@DHowett-MSFT
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DHowett-MSFT
Copy link
Contributor

You will probably need to merge in master to get the CI pipeline to work again. Sorry about that!

miniksa
miniksa requested changes Oct 3, 2019
View reviewed changes
src/renderer/dx/CustomTextLayout.cpp
_runs.clear();
_runIndex = 0;
_text = L"";

Copy link
Member

Choose a reason for hiding this comment

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

Should probably also reset _breakpoints and _numberSubstitution just for completeness.

src/renderer/dx/CustomTextLayout.h
@@ -26,6 +26,8 @@ namespace Microsoft::Console::Render
const std::basic_string_view<::Microsoft::Console::Render::Cluster> clusters,
size_t const width);

void SetData(std::basic_string_view<Cluster> const clustor);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void SetData(std::basic_string_view<Cluster> const clustor);
void SetData(std::basic_string_view<Cluster> const clusters);

typo and didn't match cpp name

src/renderer/dx/DxRenderer.cpp
_glyphCell.cx);
if (_drawingTextLayout == nullptr)
{
_drawingTextLayout = ::Microsoft::WRL::Details::Make<CustomTextLayout>(_dwriteFactory.Get(),
Copy link
Member

Choose a reason for hiding this comment

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

Why is this Microsoft::WRL::Details::Make instead of Microsoft::WRL::Make?

src/renderer/dx/DxRenderer.cpp
{
_drawingTextLayout = ::Microsoft::WRL::Details::Make<CustomTextLayout>(_dwriteFactory.Get(),
_dwriteTextAnalyzer.Get(),
_dwriteTextFormat.Get(),
Copy link
Member

Choose a reason for hiding this comment

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

Should we be using Microsoft::WRL::Make for all the other ComPtrs we make in this class too? If so, maybe we should file a follow up work item.

src/renderer/dx/DxRenderer.cpp
_dwriteFontFace.Get(),
clusters,
_glyphCell.cx);
if (_drawingTextLayout == nullptr)
Copy link
Member

Choose a reason for hiding this comment

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

Nothing is invalidating the _drawingTextLayout when the other parameters change between frames.

For instance, if _dwriteFontFace got updated between frames for a different font, we wouldn't recreate the layout. We would need to make sure that this is invalidated for any of the parameters that are now "cached" between frames inside the _drawingTextLayout.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that's what I've been thinking, too. So I left this as an idea, rather than an actual implmentation. Matter of fact, this shows as an overhead only when #2937 and #2959 is applied.

Copy link
Member

Choose a reason for hiding this comment

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

Oh. Usually if we're talking an idea and not an actual proposal to merge, we send a draft pull request and only make it real when we're trying to actually merge it. Do you intend to advance this one after #2937 and #2959 are reviewed into an actual merged change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Essentially I’d to see this one merged. But #2937 and #2959 are my main focus. They have less code but more impact on overall rendering performance. So basically yes. I’d like to see #2937 and # 2959 merged before I continue working on this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And let’s also take a look at #2960 which is even more harmless but still powerful

@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 3, 2019
@skyline75489
Copy link
Collaborator Author

After some investigation, the performance gain with this doesn't seem to be work the effort. Even though this PR eliminate the object creating and destroying, the SetData will still cost many CPU time. I'll just close it for now and leave it for further work.

@skyline75489 skyline75489 deleted the fix/reuseTextLayout branch October 21, 2019 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miniksa miniksa miniksa requested changes

@zadjii-msft zadjii-msft zadjii-msft left review comments

Assignees
No one assigned
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@skyline75489 @DHowett-MSFT @miniksa @zadjii-msft