Add cache for Skija fonts

[HeikoKlare]

No description provided.

[github-actions]

Test Results

   338 files  ±0     338 suites  ±0   2m 9s ⏱️ -13s
 3 954 tests ±0   3 670 ✅ ±0  284 💤 ±0  0 ❌ ±0 
11 690 runs  ±0  10 805 ✅ ±0  885 💤 ±0  0 ❌ ±0 

Results for commit fda6dcc. ± Comparison against base commit 2300249.

♻️ This comment has been updated with latest results.

[ddscharfe]

I can confirm that this removes the bottleneck.

However I would propose to make the cache thread safe (e.g. use ConcurrendHashMap).

[HeikoKlare]

However I would propose to make the cache thread safe (e.g. use ConcurrendHashMap).

Good pointer. The PR was originally only intended for demonstration purposes but it seems to be almost usable as is. I accordingly adapted the map to be thread-safe.

[ddscharfe]

@HeikoKlare, did you consider that this might become a memory leak, because the cache is never cleared?

[HeikoKlare]

did you consider that this might become a memory leak, because the cache is never cleared?

Yes, that might be a result if fonts are created for very many font data. However, I consider this (1) unlikely and (2) this cache is only necessary as long as we use a combination of native and Skia fonts (i.e., as long as we still use native widgets as default). Once we completely switch to Skia, the existing font caching mechanisms will directly be applied to Skia fonts instead of native fonts. That's why I think we could accept this potential leak for now, in particular since I think it would not be that easy to define a reasonable cleanup mechanism.

[ddscharfe]

did you consider that this might become a memory leak, because the cache is never cleared?

Yes, that might be a result if fonts are created for very many font data. However, I consider this (1) unlikely and (2) this cache is only necessary as long as we use a combination of native and Skia fonts (i.e., as long as we still use native widgets as default). Once we completely switch to Skia, the existing font caching mechanisms will directly be applied to Skia fonts instead of native fonts. That's why I think we could accept this potential leak for now, in particular since I think it would not be that easy to define a reasonable cleanup mechanism.

The only straightforward mechanism I can think of is to use weak references, but since this issue will disappear after the complete switch, I agree that it's acceptable to tolerate the potential leak for now.