- Notifications
You must be signed in to change notification settings - Fork6k
Defer setup of the default font manager if the embedding prefetched the font manager#28987
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 inChat. 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 theTree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| engine->SetupDefaultFontManager(); | ||
| } | ||
| }); | ||
| if (!settings_.prefetched_default_font_manager) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: consider updating the comment above to explain the guard, and where we do a last minute check to make sure it's set up.
Also consider documenting that we do it this way because it's guarded bySkOnce, so we can be confident a second call will be very quick as long as the first call has finished.
dnfield left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Change looks good, needs a test.
Perhaps something like aShellTest that passes in the new setting and verifies that the SkFontMgr has not been set up after creating the shell, but that it is created after running the engine?
It'd be interesting to see whether this helps our existing time to first frame benchmark in the framework too. I'm not sure if it will because the moto g4 has more cores than the internal device I'm testing on.
…he font managerSkFontMgr_Android can take a long time to initialize. On Android,the embedding will call FlutterJNI.nativePrefetchDefaultFontManagerearly during application startup, which will construct the globaldefault SkFontMgr on a background thread.SetupDefaultFontManager will need to wait until the prefetch hascompleted in order to obtain the default SkFontMgr. If the embeddinghas done a prefetch, then this PR moves SetupDefaultFontManager to alater phase of engine setup to make waiting less likely.Seeflutter/flutter#91045
jason-simmons commentedOct 4, 2021
Added a test - PTAL |
…etched the font manager (flutter/engine#28987)
…etched the font manager (flutter/engine#28987)
…etched the font manager (flutter/engine#28987)
…etched the font manager (flutter/engine#28987)
…etched the font manager (flutter/engine#28987)
…etched the font manager (flutter/engine#28987)
SkFontMgr_Android can take a long time to initialize. On Android,
the embedding will call FlutterJNI.nativePrefetchDefaultFontManager
early during application startup, which will construct the global
default SkFontMgr on a background thread.
SetupDefaultFontManager will need to wait until the prefetch has
completed in order to obtain the default SkFontMgr. If the embedding
has done a prefetch, then this PR moves SetupDefaultFontManager to a
later phase of engine setup to make waiting less likely.
Seeflutter/flutter#91045