-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add fast lookup path for DxFontInfo #10521
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious questions
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:
My personal conclusion: Edit: In fact we can cram all 3 values into a single uint32_t lol. |
I just saw this is already being discussed by you two... |
I like the idea of custom high-performance cache, too. Will try to implement it later. Note from Leonard:
|
053cb47
to
b44354f
Compare
b44354f
to
af2a230
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we end up removing the "check if it is the default font" case? Did it not provide a performance benefit?
@msftbot make sure @lhecker signs off |
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:
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". |
@lhecker if you agree with the suggestions, you can use the UI to commit them to Chester's branch 😄 |
std::unordered_map<FontAttributeMapKey, ::Microsoft::WRL::ComPtr<IDWriteTextFormat>> _textFormatMap; | ||
std::unordered_map<FontAttributeMapKey, ::Microsoft::WRL::ComPtr<IDWriteFontFace1>> _fontFaceMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Hello @DHowett! Because this pull request has the 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 (
|
🎉 Handy links: |
Fixes the performance regression caused by DxFontInfo.
DxFontInfo introduced in #9201