Skip to content

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.

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

Merged
merged 2 commits into from
Jun 9, 2023
Merged

Improve font performance in FamilyCollection.LookupFamily #7794

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b44dece
Improve font performance in FamilyCollection.LookupFamily
ThomasGoulet73 May 4, 2023
836c83b
Update src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/Font…
ThomasGoulet73 May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 13 additions & 16 deletions src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/FontCache/FamilyCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ ref FontStretch fontStretch
// For example, "Arial Bold"
// We will strip off the styling info (one word at a time from the end) and try to find the family name.
int indexOfSpace = -1;
System.Text.StringBuilder potentialFaceName = new System.Text.StringBuilder();
string originalFamilyName = familyName;

// Start removing off strings from the end hoping they are
// style info so as to get down to the family name.
Expand All @@ -396,12 +396,11 @@ ref FontStretch fontStretch
else
{
// store the stripped off style names to look for the specific face later.
potentialFaceName.Insert(0, familyName.AsSpan(indexOfSpace));
familyName = familyName.Substring(0, indexOfSpace);
}

fontFamilyDWrite = _fontCollection[familyName];
} while (fontFamilyDWrite == null);
} while (fontFamilyDWrite == null);


if (fontFamilyDWrite == null)
Expand All @@ -410,10 +409,12 @@ ref FontStretch fontStretch
}

// If there was styling information.
if (potentialFaceName.Length > 0)
if (familyName.Length != originalFamilyName.Length)
{
// The first character in the potentialFaceName will be a space so we need to strip it off.
Text.TextInterface.Font font = GetFontFromFamily(fontFamilyDWrite, potentialFaceName.ToString(1, potentialFaceName.Length - 1));
// To obtain the face name, we remove the family name and the next char (A space) from the original family name.
int faceNameIndex = familyName.Length + 1;
ReadOnlySpan<char> faceName = originalFamilyName.AsSpan(faceNameIndex);
Text.TextInterface.Font font = GetFontFromFamily(fontFamilyDWrite, faceName);

if (font != null)
{
Expand Down Expand Up @@ -464,19 +465,16 @@ private CompositeFontFamily LookUpUserCompositeFamily(string familyName)
/// <param name="fontFamily">The font family to look in.</param>
/// <param name="faceName">The face to look for.</param>
/// <returns>The font face if found and null if nothing was found.</returns>
private static Text.TextInterface.Font GetFontFromFamily(Text.TextInterface.FontFamily fontFamily, string faceName)
private static Text.TextInterface.Font GetFontFromFamily(Text.TextInterface.FontFamily fontFamily, ReadOnlySpan<char> faceName)
{
faceName = faceName.ToUpper(CultureInfo.InvariantCulture);

// The search that DWrite supports is a linear search.
// Look at every font face.
foreach (Text.TextInterface.Font font in fontFamily)
{
// and at every locale name this font face has.
foreach (KeyValuePair<CultureInfo, string> name in font.FaceNames)
{
string currentFontName = name.Value.ToUpper(CultureInfo.InvariantCulture);
if (currentFontName == faceName)
if (faceName.Equals(name.Value, StringComparison.OrdinalIgnoreCase))
Copy link
Contributor

@miloush miloush May 8, 2023

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.

Copy link
Contributor Author

@ThomasGoulet73 ThomasGoulet73 May 9, 2023

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 ?

Copy link
Contributor

@miloush miloush May 9, 2023 •
edited
Loading

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.

Copy link
Contributor

@wstaelens wstaelens May 9, 2023

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

{
return font;
}
Expand All @@ -488,7 +486,7 @@ private static Text.TextInterface.Font GetFontFromFamily(Text.TextInterface.Font
// thus we will start again removing words (separated by ' ') from its end and looking
// for the resulting faceName in that dictionary. So this dictionary is
// used to speed the search.
Dictionary<string, Text.TextInterface.Font> faces = new Dictionary<string, Text.TextInterface.Font>();
Dictionary<string, Text.TextInterface.Font> faces = new Dictionary<string, Text.TextInterface.Font>(StringComparer.OrdinalIgnoreCase);

//We could have merged this loop with the one above. However this will degrade the performance
//of the scenario where the user entered a correct face name (which is the common scenario).
Expand All @@ -498,8 +496,7 @@ private static Text.TextInterface.Font GetFontFromFamily(Text.TextInterface.Font
{
foreach (KeyValuePair<CultureInfo, string> name in font.FaceNames)
{
string currentFontName = name.Value.ToUpper(CultureInfo.InvariantCulture);
faces.TryAdd(currentFontName, font);
faces.TryAdd(name.Value, font);
}
}

Expand All @@ -509,8 +506,8 @@ private static Text.TextInterface.Font GetFontFromFamily(Text.TextInterface.Font

while (indexOfSpace > 0)
{
faceName = faceName.Substring(0, indexOfSpace);
if (faces.TryGetValue(faceName, out matchingFont))
faceName = faceName.Slice(0, indexOfSpace);
if (faces.TryGetValue(faceName.ToString(), out matchingFont))
{
return matchingFont;
}
Expand Down