[Linux] Fix duplicate calls to system font loading during startup #40469
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| #else | ||
| return SkFontMgr_New_Custom_Directory("/usr/share/fonts/"); | ||
| #endif | ||
| // WARNING: Not using `SkFontMgr::RefDefault()` can cause performance issues |
There was a problem hiding this comment.
Remove this comment - using SkFontMgr::RefDefault does not need a specific explanation. RefDefault is the standard choice unless the platform has a specific reason to override it.
(IIRC the previous implementation was needed because at that time Skia did not ensure that the FontConfig font manager would be preferred over the directory font manager if both are available)
There was a problem hiding this comment.
Should be fixed now, but for what it's worth the directory font manager is still the default unless the --enable-fontconfig GN flag is used (which doesn't seem to be enabled in the official build).
But I was going to propose enabling it by default on Linux (in a separate pull-request) because fontconfig seems to be faster than custom-directory on systems with lots of fonts installed (flutter/flutter#118911).
55eb98d to
1ee2601
Compare
|
auto label is removed for flutter/engine, pr: 40469, due to This PR has not met approval requirements for merging. You have project association NONE and need 1 more review(s) in order to merge this PR.
|
|
auto label is removed for flutter/engine, pr: 40469, due to Validations Fail. |
Prior to this change, the system fonts were being loaded twice, which was affecting application startup performance on Linux.
The first call was being triggered by
Engine::SetupDefaultFontManagerwhile the second one happened during the first call toSkFontMgr::RefDefault()which is usually triggered bySkTypeface::MakeFromStream(). The following stack traces show the code paths leading to these calls:First call to DirectorySystemFontLoader::loadSystemFonts
Second call to DirectorySystemFontLoader::loadSystemFonts
The root cause here is that
GetDefaultFontManager()does not set the newly initialized font manager as theSkFontMgrsingleton. This means the first call toSkFontMgr::RefDefault()has to initialize another instance of the font manager. This affects both font manager implementations on Linux: custom directory and fontconfig.This pull-request fixes that by replacing the custom
GetDefaultFontManager()implementation on Linux with a call toSkFontMgr::RefDefault(). It turns out this is working as expected and it automatically selects the correct font manager factory depending on theenable-fontconfigGN flag.This is part of a series of pull-requests that will attempt to fix some font-loading issues related to flutter/flutter#118911.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.