-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid reloading font collection for spawned engines with compatible asset managers #50897
Conversation
Nice, just tested this (two engines) and it seems to solve the issue:
|
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.
Looks great, but I'm not so familiar with the code and perhaps Jason should review this too.
assets/asset_resolver.h
Outdated
@@ -81,6 +93,10 @@ class AssetResolver { | |||
return {}; | |||
}; | |||
|
|||
virtual bool operator==(const AssetResolver& other) const = 0; | |||
|
|||
bool operator!=(const AssetResolver& other) { return !operator==(other); } |
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: declare this method as const
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.
Done
shell/common/engine.cc
Outdated
@@ -174,7 +175,8 @@ fml::WeakPtr<ImageGeneratorRegistry> Engine::GetImageGeneratorRegistry() { | |||
|
|||
bool Engine::UpdateAssetManager( | |||
const std::shared_ptr<AssetManager>& new_asset_manager) { | |||
if (asset_manager_ == new_asset_manager) { | |||
if ((asset_manager_ && new_asset_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: the parentheses around (asset_manager_ && new_asset_manager)
are not needed
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.
Done
auto label is removed for flutter/engine/50897, due to Pull request flutter/engine/50897 is not in a mergeable state. |
…mpatible asset managers (flutter/engine#50897)
…144090) flutter/engine@247971f...b7f7a84 2024-02-24 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from sO-oG6KoeFlPK2WLR... to Oac3MZ5VgZ9g3Q3cL... (flutter/engine#50946) 2024-02-24 dnfield@google.com Avoid reloading font collection for spawned engines with compatible asset managers (flutter/engine#50897) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from sO-oG6KoeFlP to Oac3MZ5VgZ9g If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@jason-simmons @jiahaog fyi
I need to figure out a test for this still but it seems to work. Hot restart is kind of a mess though, because
Engine::Restart
nulls out the asset manager it ends up reloading the font collection for every engine after a restart. But we separately need to fix hot restart, it's not very usable on the example appFixes flutter/flutter#143701