Add fast lookup path for DxFontInfo #10521

skyline75489 · 2021-06-27T04:00:38Z

Fixes the performance regression caused by DxFontInfo.

DxFontInfo introduced in #9201

skyline75489 · 2021-06-27T04:01:59Z

This feels very draft to me. I don't like most of it, but it certainly works.

Before:

After:

src/renderer/dx/DxFontInfo.h

src/renderer/dx/DxFontInfo.cpp

src/renderer/dx/DxFontRenderData.cpp

DHowett

Curious questions

lhecker · 2021-07-02T21:00:17Z

Correct me if I'm wrong, but as far as I can see the existing code that is being improved here isn't optimal in the first place. We don't have to make the hashing faster if we remove the need for expensive hashing.

Reasons:

The only place where _textFormatMap is being used is inside DxFontRenderData::TextFormatWithAttribute
_textFormatMap is cleared when the font changes and can only contain a single font ❗
_textFormatMap lookup only occurs with weight, style and stretch (let's abbreviate it as "WSS")

My personal conclusion:
The hash function for _textFormatMap only needs to hash WSS and that's it. We can improve the performance of hashing by inlining the FNV1a algorithm, which requires around 4ns for 12 bytes (the amount of data we need to hash) in my experience. If we use xxHash we can reduce it to 1-2ns.

Edit: In fact we can cram all 3 values into a single uint32_t lol.

lhecker · 2021-07-02T21:04:56Z

I just saw this is already being discussed by you two...
Well in this case I'd be in favor of creating a custom hash that doesn't specialize std::hash. That way accidental misuse can be prevented and performance guaranteed. 👍

skyline75489 · 2021-07-02T23:21:30Z

That way accidental misuse can be prevented and performance guaranteed.

I like the idea of custom high-performance cache, too. Will try to implement it later.

Note from Leonard:

you don't have to write a new class IMO
it should be enough to just create a custom struct HighPerformanceFontInfoHasher or something 😄

src/renderer/dx/DxFontRenderData.cpp

src/renderer/dx/DxFontRenderData.h

DHowett

Did we end up removing the "check if it is the default font" case? Did it not provide a performance benefit?

src/renderer/dx/DxFontRenderData.cpp

DHowett · 2021-07-09T18:55:55Z

@msftbot make sure @lhecker signs off

ghost · 2021-07-09T18:55:59Z

Hello @DHowett!

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 @lhecker

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".

DHowett · 2021-07-09T18:56:29Z

@lhecker if you agree with the suggestions, you can use the UI to commit them to Chester's branch 😄

lhecker · 2021-07-09T19:39:09Z

src/renderer/dx/DxFontRenderData.h

+        std::unordered_map<FontAttributeMapKey, ::Microsoft::WRL::ComPtr<IDWriteTextFormat>> _textFormatMap;
+        std::unordered_map<FontAttributeMapKey, ::Microsoft::WRL::ComPtr<IDWriteFontFace1>> _fontFaceMap;


I believe we can get rid of these hashmaps entirely:
As far as I can see we currently don't actually support dynamic font stretches and don't plan to add them in the foreseeable future. So we can drop those for now, allowing us to simplify our code.

With "stretch" dropped, we currently only support two kinds of bold (on/off) and two kinds of italic (on/off). It's unlikely that this will change in the foreseeable future either.
As I personally generally don't think it's a good idea to plan for the unknown (in programming), I'd like to suggest to change these line to this in a different PR:

::Microsoft::WRL::ComPtr<IDWriteTextFormat>> _textFormatMap[2][2]; ::Microsoft::WRL::ComPtr<IDWriteFontFace1>> _fontFaceMap[2][2];

If we then have for instance TextFormatWithAttribute(bool bold, bool italic) we can write _textFormatMap[bold][italic] for 0-overhead access to the format map. No allocations, or complex hashing involved. 👍

Agreed! This would be an excellent simplification.

I knew we initially designed it for font stretch, but here's what I think now: we can change it if we ever need to easily support Stretch. After all, let's go with the simplest implementation that works for any given problem unless we truly do need a complex one. 😄

I was trying to reply using email. But that email was not sent successfully.

Just want to say I am also OK with the simple implementation. It’s a good idea👍

Co-authored-by: Dustin L. Howett <dustin@howett.net>

ghost · 2021-07-09T19:43:32Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

src/renderer/dx/DxFontRenderData.cpp

ghost · 2021-07-14T17:08:08Z

🎉Windows Terminal Preview v1.10.1933.0 has been released which incorporates this pull request.:tada:

Handy links:

skyline75489 commented Jun 27, 2021

View reviewed changes

src/renderer/dx/DxFontInfo.h Show resolved Hide resolved

skyline75489 changed the title ~~Added fast lookup path for DxFontInfo~~ Add fast lookup path for DxFontInfo Jun 27, 2021

skyline75489 commented Jun 27, 2021

View reviewed changes

src/renderer/dx/DxFontInfo.cpp Outdated Show resolved Hide resolved

skyline75489 commented Jun 27, 2021

View reviewed changes

src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved

DHowett requested changes Jun 28, 2021

View reviewed changes

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 Jun 28, 2021

skyline75489 mentioned this pull request Jul 6, 2021

Extremely slow performance when processing virtual terminal sequences #10462

Open

skyline75489 force-pushed the chesterliu/dev/dxfontinfo-perf branch from 053cb47 to b44354f Compare July 6, 2021 12:36

skyline75489 marked this pull request as ready for review July 6, 2021 12:37

lhecker reviewed Jul 6, 2021

View reviewed changes

src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved

wip

87bdf7b

DHowett requested changes Jul 6, 2021

View reviewed changes

src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 6, 2021

Merge lhecker's branch

af2a230

skyline75489 force-pushed the chesterliu/dev/dxfontinfo-perf branch from b44354f to af2a230 Compare July 7, 2021 00:36

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 7, 2021

skyline75489 commented Jul 7, 2021

View reviewed changes

src/renderer/dx/DxFontRenderData.h Outdated Show resolved Hide resolved

skyline75489 added 2 commits July 7, 2021 11:58

Code format

2b3799d

Audit

8804dd7

DHowett reviewed Jul 9, 2021

View reviewed changes

DHowett approved these changes Jul 9, 2021

View reviewed changes

src/renderer/dx/DxFontRenderData.cpp Show resolved Hide resolved

src/renderer/dx/DxFontRenderData.cpp Show resolved Hide resolved

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 9, 2021

DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 9, 2021

lhecker reviewed Jul 9, 2021

View reviewed changes

Apply suggestions from code review

153b07b

Co-authored-by: Dustin L. Howett <dustin@howett.net>

lhecker approved these changes Jul 9, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 9, 2021

DHowett reviewed Jul 9, 2021

View reviewed changes

src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved

DHowett reviewed Jul 9, 2021

View reviewed changes

src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved

GOOD JOB DUSTIN YOU DID SO GOOD

d6ac578

ghost merged commit f339705 into microsoft:main Jul 9, 2021

skyline75489 deleted the chesterliu/dev/dxfontinfo-perf branch July 12, 2021 23:17

hltdev8642 mentioned this pull request Jul 16, 2021

windows terminal preview bold / cut-off fonts #10678

Closed

WSLUser mentioned this pull request Aug 23, 2021

Boost hashing performance with Robin Hood #11016

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fast lookup path for DxFontInfo #10521

Add fast lookup path for DxFontInfo #10521

skyline75489 commented Jun 27, 2021 •

edited by DHowett

skyline75489 commented Jun 27, 2021

DHowett left a comment

lhecker commented Jul 2, 2021 •

edited

lhecker commented Jul 2, 2021

skyline75489 commented Jul 2, 2021 •

edited

DHowett left a comment

DHowett commented Jul 9, 2021

ghost commented Jul 9, 2021

DHowett commented Jul 9, 2021

lhecker Jul 9, 2021 •

edited

DHowett Jul 9, 2021

skyline75489 Jul 12, 2021

ghost commented Jul 9, 2021

ghost commented Jul 14, 2021

		std::unordered_map<FontAttributeMapKey, ::Microsoft::WRL::ComPtr<IDWriteTextFormat>> _textFormatMap;
		std::unordered_map<FontAttributeMapKey, ::Microsoft::WRL::ComPtr<IDWriteFontFace1>> _fontFaceMap;

Add fast lookup path for DxFontInfo #10521

Add fast lookup path for DxFontInfo #10521

Conversation

skyline75489 commented Jun 27, 2021 • edited by DHowett

skyline75489 commented Jun 27, 2021

DHowett left a comment

Choose a reason for hiding this comment

lhecker commented Jul 2, 2021 • edited

lhecker commented Jul 2, 2021

skyline75489 commented Jul 2, 2021 • edited

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Jul 9, 2021

ghost commented Jul 9, 2021

DHowett commented Jul 9, 2021

lhecker Jul 9, 2021 • edited

Choose a reason for hiding this comment

DHowett Jul 9, 2021

Choose a reason for hiding this comment

skyline75489 Jul 12, 2021

Choose a reason for hiding this comment

ghost commented Jul 9, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Jul 14, 2021

skyline75489 commented Jun 27, 2021 •

edited by DHowett

lhecker commented Jul 2, 2021 •

edited

skyline75489 commented Jul 2, 2021 •

edited

lhecker Jul 9, 2021 •

edited

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.