Skip to content

Conversation

lindexi
Copy link
Member

@lindexi lindexi commented Sep 23, 2025

Description

As the code shows:

private Dictionary<int, ushort> CMap
{
get
{
if (_cmap == null)
{
lock (this)
{
if (_cmap == null)
{
_cmap = new Dictionary<int, ushort>();
ushort glyphIndex;
for (int codePoint = 0; codePoint <= FontFamilyMap.LastUnicodeScalar; ++codePoint)
{
if (TryGetValue(codePoint, out glyphIndex))
{
_cmap.Add(codePoint, glyphIndex);
}
}
}
}
}
return _cmap;
}
}

We can know that the cost of creating CMap is expensive. When we get the Count only, we do not need to create the CMap Dictionary.

We can get the count from FontFace directly:

Customer Impact

Performance

Regression

Testing

CI only.

Risk

Low. I do not change the behavior. The old behavior is get the count after the CMap Dictionary created, and the new behavior is get the count from FontFace directly.

Microsoft Reviewers: Open in CodeFlow

@lindexi lindexi requested a review from a team as a code owner September 23, 2025 02:43
@Copilot Copilot AI review requested due to automatic review settings September 23, 2025 02:43
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR optimizes the performance of getting the Count property in the FontFaceLayoutInfo class by avoiding the expensive creation of the CMap Dictionary when only the count is needed.

  • Introduces conditional logic to check if the CMap is already created before accessing its Count
  • Directly retrieves glyph count from FontFace when CMap is not yet initialized
  • Maintains existing behavior while improving performance for count-only operations

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Sep 23, 2025
@lindexi lindexi changed the title Avoid create the cmap Dictionary when get Count only Optimize GlyphTypeface: Avoid create the cmap Dictionary when get Count only Sep 23, 2025
@miloush
Copy link
Contributor

miloush commented Sep 23, 2025

To be fair if you just want a count you can use GlyphTypeface.GlyphCount. How did you come with this, is it in some hot path?

@h3xds1nz
Copy link
Member

h3xds1nz commented Sep 23, 2025

I might be misremembering, but doesn't CMap get created the first time you do a single text run / line anyways? Not mentioning that the generation method is slow af and should be rewritten but as miloush says, you have the prop on GlyphTypeface directly for count on public interface and FontFaceLayoutInfo does have it as well so now you're code duping baiscally.

So I'm indeed curious about the scenario where you don't end up creating it and can get away with this.

@lindexi
Copy link
Member Author

lindexi commented Sep 25, 2025

@miloush

To be fair if you just want a count you can use GlyphTypeface.GlyphCount. How did you come with this, is it in some hot path?

We just use the CharacterToGlyphMap as the Dictionary and pass it to other business code. The hot path will count the CharacterToGlyphMap Dictionary for all fonts.

@lindexi
Copy link
Member Author

lindexi commented Sep 25, 2025

@h3xds1nz As you can see, ont of the IDictionary hot path is TryGetValue method, and which do not create the CMap:

public bool TryGetValue(int key, out ushort value)
{
ushort localValue;
unsafe
{
uint uKey = checked((uint)key);
uint *pKey = &uKey;
MS.Internal.Text.TextInterface.FontFace fontFace = _font.GetFontFace();
try
{
fontFace.GetArrayOfGlyphIndices(pKey, 1, &localValue);
}
finally
{
fontFace.Release();
}
value = localValue;
}
// if a glyph is not present, index 0 is returned
return (value != 0);
}

So that, it is meaningful to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants