Bug 233972 – Move the SystemFallbackCache to FontCache

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Cameron McCormack (:heycam)]

Move the SystemFallbackCache to FontCache

[Cameron McCormack (:heycam)]

Created attachment 446291 [details]
Patch

[Cameron McCormack (:heycam)]

The Gtk/WPE failures point out that FontCacheFreeType.cpp has its own SystemFallbackCache HashMap, with different key/value types.  A bit of abstraction over these will be needed.

[Cameron McCormack (:heycam)]

Actually they seem to be doing two slightly different things.  The SystemFallbackCache that's in Font.cpp and which I'm moving out to a separate file is a per-character map of Font to the fallback Font to use.  The one in FontCacheFreeType.cpp is a cache of FcPatterns and FcFontSets that FontCache::systemFallbackForCharacters uses to avoid the overhead of doing the FcConfigSubstitute calls for the same inputs.

So I think for now we should just rename the type used in FontCacheFreeType.cpp, but it's another cache that needs to move to live on the FontCache object so that it can be thread-specific.  (Or locking added around it.)

[Cameron McCormack (:heycam)]

Created attachment 446437 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 446437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446437&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:1015
> +		32112E0F274B47CF007047A8 /* SystemFallbackCache.h in Headers */ = {isa = PBXBuildFile; fileRef = 32899FB9274B410C00855A9D /* SystemFallbackCache.h */; settings = {ATTRIBUTES = (Private, ); }; };

Can this file have the word "font" in it somewhere?

> Source/WebCore/platform/graphics/SystemFallbackCache.h:81
> +    using CharacterFallbackMap = HashMap<CharacterFallbackMapKey, Font*, CharacterFallbackMapKeyHash, CharacterFallbackMapKeyHashTraits>;
> +
> +    HashMap<const Font*, CharacterFallbackMap> m_characterFallbackMaps;

Why is this a HashMap of HashMaps?

I guess this patch is just moving code around, but we should probably at least have a bug filed about investigating whether we can use less memory and probably have better performance for this.

[Cameron McCormack (:heycam)]

(In reply to Myles C. Maxfield from comment #5)
> > Source/WebCore/platform/graphics/SystemFallbackCache.h:81
> > +    using CharacterFallbackMap = HashMap<CharacterFallbackMapKey, Font*, CharacterFallbackMapKeyHash, CharacterFallbackMapKeyHashTraits>;
> > +
> > +    HashMap<const Font*, CharacterFallbackMap> m_characterFallbackMaps;
> 
> Why is this a HashMap of HashMaps?
> 
> I guess this patch is just moving code around, but we should probably at
> least have a bug filed about investigating whether we can use less memory
> and probably have better performance for this.

Filed https://bugs.webkit.org/show_bug.cgi?id=234121.

[Radar WebKit Bug Importer]

<rdar://problem/86505855>

[Matt Woodrow]

Created attachment 459563 [details]
Rebased patch

[Matt Woodrow]

Created attachment 459564 [details]
Patch

[Matt Woodrow]

Created attachment 460997 [details]
Patch

[Matt Woodrow]

Created attachment 460998 [details]
Patch

[EWS]

'Myles Maxfield' is not a reviewer, blocking PR #None

[Matt Woodrow]

Pull request: https://github.com/WebKit/WebKit/pull/2562

[EWS]

Committed 252618@main (38edf7c001dd): <https://commits.webkit.org/252618@main>

Reviewed commits have been landed. Closing PR #2562 and removing active labels.