Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Dec 18, 2019

Problem

  • GetCharSize consumes too much CPU when the screen contains too many printable characters in IME mode

Solution

  • Cache the results of GetCharSize in a dictionary

@ghost
Copy link
Author

ghost commented Jan 31, 2020

@PavelTorgashov any chance to review this?

@Hexman768
Copy link
Collaborator

Here is an AI analysis of the code change, please take some of the potential issues into consideration, we definitely want to fix the potential for unbounded memory consumption.

What Changed

  • Added CharSizeCache class:
    • Caches character size measurements for a given font and character, aiming to avoid repeated calls to TextRenderer.MeasureText (which are expensive).
    • Uses a Dictionary<string, SizeF> where the key encodes font properties and the character.
  • Refactored GetCharSize:
    • Replaces direct measure logic with a call to CharSizeCache.GetCharSize.

Potential Issues & Review

1. Thread Safety

  • Issue: The cache (Dictionary<string, SizeF>) is static and not thread-safe. If GetCharSize is called from multiple threads (e.g., in a multi-threaded UI or background rendering), this could lead to race conditions or exceptions.
  • Recommendation: Use a concurrent dictionary (ConcurrentDictionary) or add lock protection around cache access.

2. Key Uniqueness and Font Identification

  • Issue: The cache key is constructed from font properties and the character. This should be robust, but font comparison can be tricky:
    • If two fonts have identical properties but are different instances, this approach works.
    • If the font changes after being used as a key (unlikely, but possible if mutable), cached sizes may be inaccurate.
  • Recommendation: Document this assumption or consider if it's possible for Font instances to share the same properties but behave differently.

3. Memory Usage

  • Issue: The cache can grow unbounded if many fonts and characters are used, which may impact memory usage over time.
  • Recommendation: Consider using a cache eviction strategy (e.g., LRU) or periodically clearing the cache if memory usage becomes problematic.

4. Measurement Accuracy

  • Issue: The original algorithm for measuring the width of a character is:
    Size sz2 = TextRenderer.MeasureText("<" + c.ToString() + ">", font);
    Size sz3 = TextRenderer.MeasureText("<>", font);
    return new SizeF(sz2.Width - sz3.Width + 1, font.Height);
    • This method subtracts the width of "<>" from "", presumably to get just the width of c.
    • This works for monospaced fonts but may have edge cases for proportional fonts, ligatures, or font rendering quirks.
  • Review: The caching does not change this approach, but any inaccuracies will now be reused multiple times. If the measurement algorithm is not correct for all fonts, this could propagate errors.

5. Unicode Characters / Surrogates

  • Issue: Characters outside the BMP (Basic Multilingual Plane) may be represented as surrogate pairs (two chars). The current method only accepts single char values.
  • Recommendation: If non-BMP characters are expected, consider accepting string or handling surrogate pairs.

6. Font Disposal

  • Issue: The code correctly avoids holding onto Font instances, just using their properties as the cache key.
    • This is good and prevents leaking GDI resources.

7. Performance

  • Positive: The cache should greatly improve performance if GetCharSize is called frequently with the same font/char combinations.

Summary

  • Main Issues:
    • Thread safety of the cache.
    • Potential unbounded growth of the cache.
    • Measurement algorithm may not be accurate for all fonts or characters.
  • Minor Issues:
    • Handling of surrogate pairs for non-BMP Unicode characters.

Actionable Recommendations

  1. Make the cache thread-safe (e.g., use ConcurrentDictionary).
  2. Consider cache size management if memory usage is a concern.
  3. Review the measurement algorithm for edge cases.
  4. If non-BMP Unicode characters are needed, update the function signature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants