Reduce text layout CPU usage when DWrite analysis is not needed #2959

skyline75489 · 2019-09-29T04:37:04Z

Summary of the Pull Request

It takes many CPU cycles to calculate text layout & font fallback even it's not really necessary. This may seem a little aggresive but it indeed dramatically boost performance.

References

#806

PR Checklist

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.

Detailed Description of the Pull Request / Additional comments

With the help of #2937 and this PR, vim performance is way more smooth than before.

DHowett-MSFT · 2019-09-30T17:45:49Z

/azp run

azure-pipelines · 2019-09-30T17:46:14Z

Azure Pipelines successfully started running 1 pipeline(s).

skyline75489 · 2019-10-02T12:39:16Z

Seems something wrong with the format. But I can't really tell what it is.

zadjii-msft · 2019-10-02T12:43:44Z

@skyline75489 It's probably the all_of call, if I'm spitballing. Running tools\runformat in the root of the repo should fix it for you though :)

skyline75489 · 2019-10-02T12:52:51Z

@zadjii-msft Thanks! I'll give it a try.

This PR is crucial almost the entire "make cmatrix great again" movement. Would love to see it merged.

zadjii-msft

This looks reasonable to me, but I don't think I want this in w/o @miniksa's sign off on it, since he knows the renderer best.

zadjii-msft · 2019-10-02T12:57:29Z

src/renderer/dx/CustomTextLayout.cpp

-        // Perform our custom font fallback analyzer that mimics the pattern of the real analyzers.
-        RETURN_IF_FAILED(_AnalyzeFontFallback(this, 0, textLength));
+        const bool whiteSpacesOnly = std::all_of(
+            _text.begin(), _text.end(), [](wchar_t c) noexcept {


Out of curiosity, can we just pass &iswspace as the function? (my c++17-fu is not strong so this is mostly an educational question).

(and the same below)

Yeal it could be &iswspace. It's just initially the condition here is a little more than a simple function. So I left it here.

I'd also love to here from some one who knows this more. I'm afraid some of the optimization could be to aggresive. So no need to rush it.

miniksa · 2019-10-11T22:46:51Z

For the whitespace part:

Using the CRT function iswspace is uncomfortable to me because it is doing a locale detection in a way that is completely different and potentially out of sync from whatever locale settings we're using in the rest of DirectWrite.
We honestly shouldn't be dispatching draw requests for whitespace at all. This is the fault of

terminal/src/renderer/dx/DxRenderer.cpp

Line 734 in b9233c0

FAIL_FAST_IF_FAILED(InvalidateAll());

which I put in there to always force full frame drawing until we could just get things to work. Restore differential drawing to DirectWrite renderer #778 to restore differential drawing would remove that line and eliminate a grand majority of whitespace drawing through the existing dirty region tracking.

For the ascii only part:

Again, not a fan of using the CRT functions.
If this method's really expensive, we can probably avoid it another way. I'm pretty sure _analyzer has a GetTextComplexity() method that would probably indicate to us whether or not we need to go into the whole expensive calculations or not. For that matter, the complexity function might be able to gate all of these calls.

I would prefer to always lean on further portions of the DirectWrite framework to analyze the text, determine complexity, or figure out which regions we can discard before calling an expensive function. I really don't want to mix in other technologies like the CRT here. If we don't know the right way to do something in DWrite, I'd rather lean on our internal contacts on those teams to help us figure it out than try to reverse it with a mix of other technologies.

Overall, though, I think this particular CPU concern is just majorly an artifact of #778 being incomplete.

skyline75489 · 2019-10-12T03:30:14Z

@miniksa I agree with you on that #778 is the ultimate solution for overall performance. However, I'd argue that this optimization is still needed even #778 is complete. There are still many situations that requires entire screen redrawing(e.g. vim, cat, and less). Differential drawing won't optimize away the CPU burden with all those DWrite analysis.

I'm not a DWrite expert and I chose CRT on this one. Seems GetTextComplexity() also works great on this one.

skyline75489 · 2019-10-12T08:27:19Z

The concern I have on this, is what will GetTextComplexity behave under more sophiscated condition. For example, different locale and font with ligatures support. If we can trust GetTextComplexity on this, I think it's safe to use it for a short cut.

skyline75489 · 2019-10-12T11:47:38Z

I found a catch. If I open cmd.exe the Chinese character is not correctly rendered.

skyline75489 · 2019-10-12T12:41:01Z

I added the check of text read length to eliminate the issue with cmd.exe.

miniksa

I'm good with this now. Thanks!

DHowett-MSFT

I didn't realize that we backed out our complexity analysis. Yeah, this is awesome. Thank you!

ghost · 2019-10-23T19:55:35Z

🎉Windows Terminal Preview v0.6.2951.0 has been released which incorporates this pull request.:tada:

Handy links:

madig · 2019-10-25T10:23:54Z

Hrm, I remember someone saying somewhere that Safari uses a similar trick to save on layouting and that it can lead to tricky edge-cases with font features. A terminal is probably less likely to deal with OpenType Layout data in fonts, so it might be okay 🤔

@skyline75489

## Summary of the Pull Request As the title suggests, this PR will make CustomTextLayout skip glyph shaping analysis when the entire text is detected as simple. ## References My main reference is [DirectX Factor - Who’s Afraid of Glyph Runs?](https://docs.microsoft.com/en-us/archive/msdn-magazine/2013/november/directx-factor-who%e2%80%99s-afraid-of-glyph-runs) And also #2959 ## PR Checklist * [x] Closes @skyline75489's continuous drive for perf gainz. * [x] CLA signed. * [x] Manual tests. * [x] Nah on docs. * [x] Discussed with core contributors in this PR. ## Detailed Description of the Pull Request / Additional comments This can be seen as a followup of #2959. The idea is the same: make use of simple text (which makes up I think 95% of all terminal text) as much as possible. The performance boost is huge. Cacafire is actually on fire this time (and remember I'm using 4K!). The frame rate is also boosted since more CPU time can be used for actual drawing. Before: ![图片](https://user-images.githubusercontent.com/4710575/82913277-b21c3c00-9fa0-11ea-8785-a14b347bbcbd.png) After: ![图片](https://user-images.githubusercontent.com/4710575/82912969-4afe8780-9fa0-11ea-8795-92617dde822f.png) ## Validation Steps Performed Manually validated.

skyline75489 changed the title ~~Reduce text layou CPU usage when run is whitespace and alphanum only~~ Reduce text layout CPU usage when run is whitespace and alphanum only Sep 29, 2019

skyline75489 changed the title ~~Reduce text layout CPU usage when run is whitespace and alphanum only~~ Reduce text layout CPU usage when run is whitespace or ASCII only Sep 30, 2019

zadjii-msft requested a review from miniksa October 1, 2019 14:11

zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-Performance Performance-related issue labels Oct 1, 2019

skyline75489 force-pushed the fix/textLayoutPerf branch 2 times, most recently from 67b047d to e566a4a Compare October 2, 2019 12:24

zadjii-msft reviewed Oct 2, 2019

View reviewed changes

skyline75489 mentioned this pull request Oct 4, 2019

Reuse text layout for performance #2980

Closed

5 tasks

skyline75489 force-pushed the fix/textLayoutPerf branch from 5300144 to 9547cbd Compare October 12, 2019 03:34

skyline75489 changed the title ~~Reduce text layout CPU usage when run is whitespace or ASCII only~~ Reduce text layout CPU usage when DWrite analysis is not needed Oct 12, 2019

Added shortcut when DWrite analysis in not needed

72712da

skyline75489 force-pushed the fix/textLayoutPerf branch from 9547cbd to 72712da Compare October 12, 2019 03:54

Added check for text read length

ab372f6

miniksa approved these changes Oct 17, 2019

View reviewed changes

DHowett-MSFT approved these changes Oct 17, 2019

View reviewed changes

DHowett-MSFT merged commit 6f7ad99 into microsoft:master Oct 17, 2019

skyline75489 deleted the fix/textLayoutPerf branch October 21, 2019 07:01

skyline75489 mentioned this pull request Nov 11, 2019

Restore differential drawing to DirectWrite renderer #778

Closed

skyline75489 mentioned this pull request May 26, 2020

Skip glyph shaping analysis when the entire text is simple #6206

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce text layout CPU usage when DWrite analysis is not needed #2959

Reduce text layout CPU usage when DWrite analysis is not needed #2959

skyline75489 commented Sep 29, 2019

DHowett-MSFT commented Sep 30, 2019

azure-pipelines bot commented Sep 30, 2019

skyline75489 commented Oct 2, 2019

zadjii-msft commented Oct 2, 2019

skyline75489 commented Oct 2, 2019

zadjii-msft left a comment

zadjii-msft Oct 2, 2019

skyline75489 Oct 2, 2019

miniksa commented Oct 11, 2019

skyline75489 commented Oct 12, 2019

skyline75489 commented Oct 12, 2019

skyline75489 commented Oct 12, 2019

skyline75489 commented Oct 12, 2019

miniksa left a comment

DHowett-MSFT left a comment

ghost commented Oct 23, 2019

madig commented Oct 25, 2019

Reduce text layout CPU usage when DWrite analysis is not needed #2959

Reduce text layout CPU usage when DWrite analysis is not needed #2959

Conversation

skyline75489 commented Sep 29, 2019

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

DHowett-MSFT commented Sep 30, 2019

azure-pipelines bot commented Sep 30, 2019

skyline75489 commented Oct 2, 2019

zadjii-msft commented Oct 2, 2019

skyline75489 commented Oct 2, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Oct 2, 2019

Choose a reason for hiding this comment

skyline75489 Oct 2, 2019

Choose a reason for hiding this comment

miniksa commented Oct 11, 2019

skyline75489 commented Oct 12, 2019

skyline75489 commented Oct 12, 2019

skyline75489 commented Oct 12, 2019

skyline75489 commented Oct 12, 2019

miniksa left a comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

ghost commented Oct 23, 2019

madig commented Oct 25, 2019