Bug 233972 - Move the SystemFallbackCache to FontCache
Summary: Move the SystemFallbackCache to FontCache
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Cameron McCormack (:heycam)
URL:
Keywords: InRadar
Depends on:
Blocks: 233488
  Show dependency treegraph
 
Reported: 2021-12-07 21:59 PST by Cameron McCormack (:heycam)
Modified: 2022-05-25 06:27 PDT (History)
12 users (show)

See Also:


Attachments
Patch (22.59 KB, patch)
2021-12-07 21:59 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch (25.23 KB, patch)
2021-12-08 15:24 PST, Cameron McCormack (:heycam)
mmaxfield: review+
Details | Formatted Diff | Diff
Rebased patch (22.58 KB, patch)
2022-05-18 18:51 PDT, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (25.94 KB, patch)
2022-05-18 20:30 PDT, Matt Woodrow
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2021-12-07 21:59:17 PST
Move the SystemFallbackCache to FontCache
Comment 1 Cameron McCormack (:heycam) 2021-12-07 21:59:49 PST
Created attachment 446291 [details]
Patch
Comment 2 Cameron McCormack (:heycam) 2021-12-08 13:59:40 PST
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.
Comment 3 Cameron McCormack (:heycam) 2021-12-08 15:07:04 PST
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.)
Comment 4 Cameron McCormack (:heycam) 2021-12-08 15:24:26 PST
Created attachment 446437 [details]
Patch
Comment 5 Myles C. Maxfield 2021-12-08 21:00:55 PST
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.
Comment 6 Cameron McCormack (:heycam) 2021-12-09 20:46:24 PST
(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.
Comment 7 Radar WebKit Bug Importer 2021-12-14 22:00:34 PST
<rdar://problem/86505855>
Comment 8 Matt Woodrow 2022-05-18 18:51:10 PDT
Created attachment 459563 [details]
Rebased patch
Comment 9 Matt Woodrow 2022-05-18 20:30:01 PDT
Created attachment 459564 [details]
Patch