-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Improve font performance in FamilyCollection.LookupFamily #7794
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 font performance in FamilyCollection.LookupFamily #7794
Conversation
src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/FontCache/FamilyCollection.cs
Outdated
Show resolved
Hide resolved
| { | ||
| string currentFontName = name.Value.ToUpper(CultureInfo.InvariantCulture); | ||
| if (currentFontName == faceName) | ||
| if (faceName.Equals(name.Value, StringComparison.OrdinalIgnoreCase)) |
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 am concerned about switching from invariant to ordinal. Using NLS, this no longer matches for example "ss" with "ß".
Not only that, I don't quite understand why the CultureInfo associated with that face name (i.e. name.Key) shouldn't be used for this comparison?
Applies to the dictionary too.
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'm not sure it is that big of a concern, it was already using ordinal comparison but using InvariantCulture for the "ignore case" part. So the only thing that this PR could change is in the case where faceName.Equals(name.Value, StringComparison.OrdinalIgnoreCase) != faceName.ToUpper(CultureInfo.InvariantCulture).Equals(name.Value.ToUpper(CultureInfo.InvariantCulture), StringComparison.Ordinal). Are you aware of any cases where this would apply ?
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.
Good point! But yes, I am. For NLS, the Deseret characters (starting with U+10400). For ICU, the Vithkuqi characters (starting with U+10570).
You convinced me though that the change is not worse than the existing behavior. I disagree that face names should be compared using ordinal rules in principle, but that is a separate issue.
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.
just be aware of this: http://www.moserware.com/2008/02/does-your-code-pass-turkey-test.html
(and I thought temporary fonts can have crazy (random) names).
…Cache/FamilyCollection.cs Co-authored-by: halgab <24685886+halgab@users.noreply.github.com>
|
LGTM |
|
Awesome |
|
@ThomasGoulet73 this is a good perf improvement. Thanks for your constant efforts. |
|
Thank you @dipeshmsft |
Description
I improved performance and allocations by using spans and using ordinal ignore case comparison instead of allocating new strings with ToUpper.
Here's the result of my benchmark (I'll post the code soon):
Customer Impact
Better perf.
Regression
No.
Testing
I ran FamilyCollection.LookupFamily with different combinations to make sure that the result was the same.
Risk
Low.
Microsoft Reviewers: Open in CodeFlow